Re: [PATCH v2 1/4] md/raid10: cleanup wait_barrier()
From: Logan Gunthorpe <logang@deltatee.com>
Date: 2022-09-14 16:17:04
Also in:
lkml
On 2022-09-13 19:49, Yu Kuai wrote:
quoted hunk ↗ jump to hunk
From: Yu Kuai <redacted> Currently the nasty condition is wait_barrier() is hard to read. This patch factor out the condition into a function. There are no functional changes. Signed-off-by: Yu Kuai <redacted> --- drivers/md/raid10.c | 56 ++++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 24 deletions(-)diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index 64d6e4cd8a3a..56458a53043d 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c@@ -957,44 +957,52 @@ static void lower_barrier(struct r10conf *conf) wake_up(&conf->wait_barrier); } +static bool stop_waiting_barrier(struct r10conf *conf) +{ + /* barrier is dropped */ + if (!conf->barrier) + return true; + + /* + * If there are already pending requests (preventing the barrier from + * rising completely), and the pre-process bio queue isn't empty, then + * don't wait, as we need to empty that queue to get the nr_pending + * count down. + */ + if (atomic_read(&conf->nr_pending)) { + struct bio_list *bio_list = current->bio_list;
I'd probably just put the bio_list declaration at the top of this function, then the nested if statements are not necessary. The compiler should be able to optimize the access just fine.
if (conf->barrier) {
- struct bio_list *bio_list = current->bio_list;
- conf->nr_waiting++;
- /* Wait for the barrier to drop.
- * However if there are already pending
- * requests (preventing the barrier from
- * rising completely), and the
- * pre-process bio queue isn't empty,
- * then don't wait, as we need to empty
- * that queue to get the nr_pending
- * count down.
- */
/* Return false when nowait flag is set */
if (nowait) {
ret = false;
} else {
+ conf->nr_waiting++;Technically speaking, I think moving nr_waiting counts as a functional change. As best as I can see it is correct, but it should probably be at least mentioned in the commit message, or maybe done as a separate commit with it's own justification. That way if it causes problems down the road, a bisect will make the issue clearer. Thanks, Logan