| Message ID | 20220929074745.103073-3-p.raghav@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | plugging cleanup v3 | expand |
On 9/29/22 16:47, Pankaj Raghav wrote: > Use blk_mq_plug() wrapper to get the plug instead of directly accessing > it in the block layer. > > Either of the changes should not have led to a bug in zoned devices: > > - blk_execute_rq_nowait: > Only passthrough requests can use this function, and plugging can be > performed on those requests in zoned devices. So no issues directly > accessing the plug. > > - blk_flush_plug in bio_poll: > As we don't plug the requests that require a zone lock in the first > place, flushing should not have any impact. So no issues directly > accessing the plug. > > This is just a cleanup patch to use this wrapper to get the plug > consistently across the block layer. > > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > block/blk-core.c | 2 +- > block/blk-mq.c | 6 ++++-- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index 203be672da52..d0e97de216db 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -850,7 +850,7 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags) > !test_bit(QUEUE_FLAG_POLL, &q->queue_flags)) > return 0; > > - blk_flush_plug(current->plug, false); > + blk_flush_plug(blk_mq_plug(bio), false); > > if (bio_queue_enter(bio)) > return 0; > diff --git a/block/blk-mq.c b/block/blk-mq.c > index c11949d66163..5bf245c4bf0a 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -1209,12 +1209,14 @@ static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq) > */ > void blk_execute_rq_nowait(struct request *rq, bool at_head) > { > + struct blk_plug *plug = blk_mq_plug(rq->bio); > + > WARN_ON(irqs_disabled()); > WARN_ON(!blk_rq_is_passthrough(rq)); > > blk_account_io_start(rq); > - if (current->plug) > - blk_add_rq_to_plug(current->plug, rq); > + if (plug) > + blk_add_rq_to_plug(plug, rq); > else > blk_mq_sched_insert_request(rq, at_head, true, false); > }
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 9/29/22 1:47 AM, Pankaj Raghav wrote: > Use blk_mq_plug() wrapper to get the plug instead of directly accessing > it in the block layer. > > Either of the changes should not have led to a bug in zoned devices: > > - blk_execute_rq_nowait: > Only passthrough requests can use this function, and plugging can be > performed on those requests in zoned devices. So no issues directly > accessing the plug. > > - blk_flush_plug in bio_poll: > As we don't plug the requests that require a zone lock in the first > place, flushing should not have any impact. So no issues directly > accessing the plug. > > This is just a cleanup patch to use this wrapper to get the plug > consistently across the block layer. While I did suggest to make this consistent and in principle it's the right thing to do, it also irks me to add extra checks to paths where we know that it's just extra pointless code. Maybe we can just comment these two spots? Basically each of the sections above could just go into the appropriate file.
>> Either of the changes should not have led to a bug in zoned devices: >> >> - blk_execute_rq_nowait: >> Only passthrough requests can use this function, and plugging can be >> performed on those requests in zoned devices. So no issues directly >> accessing the plug. >> >> - blk_flush_plug in bio_poll: >> As we don't plug the requests that require a zone lock in the first >> place, flushing should not have any impact. So no issues directly >> accessing the plug. >> >> This is just a cleanup patch to use this wrapper to get the plug >> consistently across the block layer. > > While I did suggest to make this consistent and in principle it's > the right thing to do, it also irks me to add extra checks to paths > where we know that it's just extra pointless code. Maybe we can > just comment these two spots? Basically each of the sections above > could just go into the appropriate file. > The checks should go away, and the plug could be inlined by the compiler if we don't have CONFIG_BLK_DEV_ZONED. Otherwise, I do agree with you that it is a pointless check. I am fine with either, though I prefer what this patch is doing. So if you feel strongly against calling the blk_mq_plug function, I can turn this patch into just adding comments as you suggested.
On 9/29/22 7:41 AM, Pankaj Raghav wrote: >>> Either of the changes should not have led to a bug in zoned devices: >>> >>> - blk_execute_rq_nowait: >>> Only passthrough requests can use this function, and plugging can be >>> performed on those requests in zoned devices. So no issues directly >>> accessing the plug. >>> >>> - blk_flush_plug in bio_poll: >>> As we don't plug the requests that require a zone lock in the first >>> place, flushing should not have any impact. So no issues directly >>> accessing the plug. >>> >>> This is just a cleanup patch to use this wrapper to get the plug >>> consistently across the block layer. >> >> While I did suggest to make this consistent and in principle it's >> the right thing to do, it also irks me to add extra checks to paths >> where we know that it's just extra pointless code. Maybe we can >> just comment these two spots? Basically each of the sections above >> could just go into the appropriate file. >> > The checks should go away, and the plug could be inlined by the > compiler if we don't have CONFIG_BLK_DEV_ZONED. Otherwise, I do agree > with you that it is a pointless check. But that's my general complaint with the argument that "it doesn't matter if this feature isn't configured" - distros enable features. Anything that uses IS_ENABLED() is handy for testing, but assuming it all pretty much gets enabled in the distro kernel, it does absolutely nothing to help the added overhead. It's something that gets done to create the illusion that an added feature CAN have zero core overhead, while in reality that's utterly wrong. > I am fine with either, though I prefer what this patch is doing. So if > you feel strongly against calling the blk_mq_plug function, I can turn > this patch into just adding comments as you suggested. I think we should. I can pick patch 1 here, and then you can send a patch 2 for that when you have time.
On 2022-09-29 15:45, Jens Axboe wrote: > On 9/29/22 7:41 AM, Pankaj Raghav wrote: >>>> Either of the changes should not have led to a bug in zoned devices: >>>> >>>> - blk_execute_rq_nowait: >>>> Only passthrough requests can use this function, and plugging can be >>>> performed on those requests in zoned devices. So no issues directly >>>> accessing the plug. >>>> >>>> - blk_flush_plug in bio_poll: >>>> As we don't plug the requests that require a zone lock in the first >>>> place, flushing should not have any impact. So no issues directly >>>> accessing the plug. >>>> >>>> This is just a cleanup patch to use this wrapper to get the plug >>>> consistently across the block layer. >>> >>> While I did suggest to make this consistent and in principle it's >>> the right thing to do, it also irks me to add extra checks to paths >>> where we know that it's just extra pointless code. Maybe we can >>> just comment these two spots? Basically each of the sections above >>> could just go into the appropriate file. >>> >> The checks should go away, and the plug could be inlined by the >> compiler if we don't have CONFIG_BLK_DEV_ZONED. Otherwise, I do agree >> with you that it is a pointless check. > > But that's my general complaint with the argument that "it doesn't > matter if this feature isn't configured" - distros enable features. > Anything that uses IS_ENABLED() is handy for testing, but assuming it > all pretty much gets enabled in the distro kernel, it does absolutely > nothing to help the added overhead. It's something that gets done to > create the illusion that an added feature CAN have zero core overhead, > while in reality that's utterly wrong. > Got it! >> I am fine with either, though I prefer what this patch is doing. So if >> you feel strongly against calling the blk_mq_plug function, I can turn >> this patch into just adding comments as you suggested. > > I think we should. I can pick patch 1 here, and then you can send a > patch 2 for that when you have time. > I will do it right away as a separate patch! Thanks.
diff --git a/block/blk-core.c b/block/blk-core.c index 203be672da52..d0e97de216db 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -850,7 +850,7 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags) !test_bit(QUEUE_FLAG_POLL, &q->queue_flags)) return 0; - blk_flush_plug(current->plug, false); + blk_flush_plug(blk_mq_plug(bio), false); if (bio_queue_enter(bio)) return 0; diff --git a/block/blk-mq.c b/block/blk-mq.c index c11949d66163..5bf245c4bf0a 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1209,12 +1209,14 @@ static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq) */ void blk_execute_rq_nowait(struct request *rq, bool at_head) { + struct blk_plug *plug = blk_mq_plug(rq->bio); + WARN_ON(irqs_disabled()); WARN_ON(!blk_rq_is_passthrough(rq)); blk_account_io_start(rq); - if (current->plug) - blk_add_rq_to_plug(current->plug, rq); + if (plug) + blk_add_rq_to_plug(plug, rq); else blk_mq_sched_insert_request(rq, at_head, true, false); }
Use blk_mq_plug() wrapper to get the plug instead of directly accessing it in the block layer. Either of the changes should not have led to a bug in zoned devices: - blk_execute_rq_nowait: Only passthrough requests can use this function, and plugging can be performed on those requests in zoned devices. So no issues directly accessing the plug. - blk_flush_plug in bio_poll: As we don't plug the requests that require a zone lock in the first place, flushing should not have any impact. So no issues directly accessing the plug. This is just a cleanup patch to use this wrapper to get the plug consistently across the block layer. Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> --- block/blk-core.c | 2 +- block/blk-mq.c | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-)