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. -- Jens Axboe
