[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v7 4/9] block: treat BDRV_REQ_ALLOCATE as serial

From: Anton Nefedov
Subject: Re: [Qemu-block] [PATCH v7 4/9] block: treat BDRV_REQ_ALLOCATE as serialising
Date: Wed, 31 Jan 2018 20:11:27 +0300
User-agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2

On 31/1/2018 6:11 PM, Alberto Garcia wrote:
On Thu 18 Jan 2018 06:49:02 PM CET, Anton Nefedov wrote:

-static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
+static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self,
+                                                   bool nowait)

It's a bit confusing to have a function called wait_foo() with a
parameter that says "don't wait"...

How about

      check_serialising_requests(BdrvTrackedRequest *self, bool wait)

I think it might be more important to emphasize in the name that the
function _might_ wait.

i.e. it feels worse to read
  check_serialising_requests(req, true);
when one needs to follow the function to find out that it might yield.

Personally I'd vote for

    static int check_or_wait_serialising_requests(
        BdrvTrackedRequest *self, bool wait) {}

and maybe even:

    static int check_serialising_requests(BdrvTrackedRequest *self) {
        return check_or_wait_serialising_requests(self, false);

    static int wait_serialising_requests(BdrvTrackedRequest *self) {
        return check_or_wait_serialising_requests(self, true);

-    waited = wait_serialising_requests(req);
+    waited = wait_serialising_requests(req, flags & BDRV_REQ_ALLOCATE);
+    if (waited && flags & BDRV_REQ_ALLOCATE) {
+        return -EAGAIN;
+    }

I find this more readable (even if not strictly necessary):

        if (waited && (flags & BDRV_REQ_ALLOCATE)) {


None of my two comments are blockers, though, so

Reviewed-by: Alberto Garcia <address@hidden>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]