| Message ID | 20230510085208epcms2p52a6dec8da80152ec2101f11ce2ea5321@epcms2p5 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Change the integrity configuration method in block | expand |
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ẏ
>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? > Yes. I will fix it. >> +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. There are many conditional sentences like that, so I left them for unity, If it's okay to change, I'll do so. >> +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ẏ OK. I will drop this. Best Regards, Jinyoung.
diff --git a/block/blk-integrity.c b/block/blk-integrity.c index d4e9b4556d14..03a85e1f6d2e 100644 --- a/block/blk-integrity.c +++ b/block/blk-integrity.c @@ -184,19 +184,43 @@ bool blk_integrity_merge_rq(struct request_queue *q, struct request *req, return true; } +static inline bool blk_integrity_bypass_check(struct request *req, + struct bio *bio) +{ + return blk_integrity_rq(req) == 0 && bio_integrity(bio) == NULL; +} + +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); +} + bool blk_integrity_merge_bio(struct request_queue *q, struct request *req, struct bio *bio) { int nr_integrity_segs; struct bio *next = bio->bi_next; - if (blk_integrity_rq(req) == 0 && bio_integrity(bio) == NULL) + if (blk_integrity_bypass_check(req, bio)) return true; - if (blk_integrity_rq(req) == 0 || bio_integrity(bio) == NULL) - return false; - - if (bio_integrity(req->bio)->bip_flags != bio_integrity(bio)->bip_flags) + if (!__blk_integrity_mergeable(q, req, bio)) return false; bio->bi_next = NULL; diff --git a/block/blk-merge.c b/block/blk-merge.c index 65e75efa9bd3..8509f468d6d4 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -611,9 +611,6 @@ static inline int ll_new_hw_segment(struct request *req, struct bio *bio, if (!blk_cgroup_mergeable(req, bio)) goto no_merge; - if (blk_integrity_merge_bio(req->q, req, bio) == false) - goto no_merge; - /* discard request merge won't add new segment */ if (req_op(req) == REQ_OP_DISCARD) return 1; @@ -621,6 +618,10 @@ static inline int ll_new_hw_segment(struct request *req, struct bio *bio, if (req->nr_phys_segments + nr_phys_segs > blk_rq_get_max_segments(req)) goto no_merge; + /* This will merge integrity segments */ + if (!blk_integrity_merge_bio(req->q, req, bio)) + goto no_merge; + /* * This will form the start of a new hw segment. Bump both * counters. @@ -934,7 +935,7 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio) return false; /* only merge integrity protected bio into ditto rq */ - if (blk_integrity_merge_bio(rq->q, rq, bio) == false) + if (!blk_integrity_mergeable(rq->q, rq, bio)) return false; /* Only merge if the crypt contexts are compatible */ diff --git a/block/blk.h b/block/blk.h index dd7cbb57ce43..b7677a5bdff1 100644 --- a/block/blk.h +++ b/block/blk.h @@ -202,6 +202,8 @@ static inline bool bio_integrity_endio(struct bio *bio) bool blk_integrity_merge_rq(struct request_queue *, struct request *, struct request *); +bool blk_integrity_mergeable(struct request_queue *rq, struct request *r, + struct bio *b); bool blk_integrity_merge_bio(struct request_queue *, struct request *, struct bio *); @@ -234,6 +236,11 @@ static inline bool blk_integrity_merge_rq(struct request_queue *rq, { return true; } +static inline bool blk_integrity_mergeable(struct request_queue *rq, + struct request *r, struct bio *b) +{ + return true; +} static inline bool blk_integrity_merge_bio(struct request_queue *rq, struct request *r, struct bio *b) {
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. Cc: Christoph Hellwig <hch@lst.de> Cc: Martin K. Petersen <martin.petersen@oracle.com> Fixes: 4eaf99beadce ("block: Don't merge requests if integrity flags differ") Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com> --- block/blk-integrity.c | 34 +++++++++++++++++++++++++++++----- block/blk-merge.c | 9 +++++---- block/blk.h | 7 +++++++ 3 files changed, 41 insertions(+), 9 deletions(-)