[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230512135136.GD32242@lst.de>
Date: Fri, 12 May 2023 15:51:36 +0200
From: "hch@....de" <hch@....de>
To: Jinyoung CHOI <j-young.choi@...sung.com>
Cc: "axboe@...nel.dk" <axboe@...nel.dk>,
"kbusch@...nel.org" <kbusch@...nel.org>, "hch@....de" <hch@....de>,
"sagi@...mberg.me" <sagi@...mberg.me>,
"jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
"martin.petersen@...cle.com" <martin.petersen@...cle.com>,
"johannes.thumshirn@....com" <johannes.thumshirn@....com>,
"kch@...dia.com" <kch@...dia.com>,
"willy@...radead.org" <willy@...radead.org>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 05/14] block: blk-merge: fix to add the number of
integrity segments to the request twice
The subject looks a bit odd, I think you're trying to say:
"do not add the number of integrity segments to the request twice"
based on the actual patch, is this correct?
> blk_integrity_merge_bio() not only performs conditional tests, but also
> updates the integrity segment information of request.
> It can be called twice when merging the bio into an existing request.
>
> bio_attempt_bio_merge() or blk_mq_sched_try_merge()
> blk_rq_merge_ok()
> blk_integrity_merge_bio() - 1
> bio_attemp_{back|front}_merge()
> ll_{back|front}_merge_fn()
> ll_new_hw_segments()
> blk_integrity_merge_bio() - 2
>
> The part of checking the conditions and the code to update the
> information of the actual request were separated. At this time, the
> ll_back_merge_fn was called by passth-path, so the condition check was
> called by all the separated functions.
>
> And after success in blk_integrity_merge_bio(), the information of the
> request may be wrong if it is impossible to merge due to other
> conditional tests. Thus, it was changed to be called immediately before
> merging the bio's segments.
> +static inline bool blk_integrity_bypass_check(struct request *req,
> + struct bio *bio)
> +{
> + return blk_integrity_rq(req) == 0 && bio_integrity(bio) == NULL;
> +}
No need for the explicit comparisms, this could just be:
return !blk_integrity_rq(req) && !bio_integrity(bio);
and given that it just has two callers I'm not sure the helper is
all that useful to start with.
> +static bool __blk_integrity_mergeable(struct request_queue *q,
> + struct request *req, struct bio *bio)
> +{
> + if (blk_integrity_rq(req) == 0 || bio_integrity(bio) == NULL)
> + return false;
> +
> + if (bio_integrity(req->bio)->bip_flags != bio_integrity(bio)->bip_flags)
> + return false;
> +
> + return true;
> +}
> +
> +bool blk_integrity_mergeable(struct request_queue *q, struct request *req,
> + struct bio *bio)
> +{
> + if (blk_integrity_bypass_check(req, bio))
> + return true;
> +
> + return __blk_integrity_mergeable(q, req, bio);
> +}
Similarly here, I'm not even sure we need all these helpers. I supect
the code would become more readable by dropping these helpers and just
making the checks explicitlẏ
Powered by blists - more mailing lists