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

[Anton Nefedov]

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)) {

Done!

> None of my two comments are blockers, though, so
>
> Reviewed-by: Alberto Garcia <address@hidden>
>
> Berto