|
From: | Anton Nefedov |
Subject: | Re: [Qemu-devel] [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)) {
Done!
None of my two comments are blockers, though, so Reviewed-by: Alberto Garcia <address@hidden> Berto
[Prev in Thread] | Current Thread | [Next in Thread] |