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

[Alberto Garcia]

On Wed 31 Jan 2018 06:11:27 PM CET, Anton Nefedov wrote:
> 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);
>      }

You're right. Either approach works for me though, also keeping the
current solution, wait_serialising_requests(req, true).

Berto