Re: [RESEND PATCH v5 2/2] bio: add limit_bio_size sysfs

[Greg KH]

On Wed, Apr 07, 2021 at 10:21:17AM +0900, Changheun Lee wrote:
> > On 3/16/21 12:44 AM, Changheun Lee wrote:
> > > Add limit_bio_size block sysfs node to limit bio size.
> > > Queue flag QUEUE_FLAG_LIMIT_BIO_SIZE will be set if limit_bio_size is set.
> > > And bio max size will be limited by queue max sectors via
> > > QUEUE_FLAG_LIMIT_BIO_SIZE set.
> > > 
> > > Signed-off-by: Changheun Lee <nanich.lee@xxxxxxxxxxx>
> > > ---
> > >  Documentation/ABI/testing/sysfs-block | 10 ++++++++++
> > >  Documentation/block/queue-sysfs.rst   |  7 +++++++
> > >  block/blk-sysfs.c                     |  3 +++
> > >  3 files changed, 20 insertions(+)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/testing/sysfs-block
> > > index e34cdeeeb9d4..86a7b15410cf 100644
> > > --- a/Documentation/ABI/testing/sysfs-block
> > > +++ b/Documentation/ABI/testing/sysfs-block
> > > @@ -316,3 +316,13 @@ Description:
> > >  		does not complete in this time then the block driver timeout
> > >  		handler is invoked. That timeout handler can decide to retry
> > >  		the request, to fail it or to start a device recovery strategy.
> > > +
> > > +What:		/sys/block/<disk>/queue/limit_bio_size
> > > +Date:		Feb, 2021
> > > +Contact:	Changheun Lee <nanich.lee@xxxxxxxxxxx>
> > > +Description:
> > > +		(RW) Toggle for set/clear QUEUE_FLAG_LIMIT_BIO_SIZE queue flag.
> > > +		Queue flag QUEUE_FLAG_LIMIT_BIO_SIZE will be set if limit_bio_size
> > > +		is set. And bio max size will be limited by queue max sectors.
> > > +		QUEUE_FLAG_LIMIT_BIO_SIZE will be cleared if limit_bio_size is
> > > +		cleard. And limit of bio max size will be cleard.
> > > diff --git a/Documentation/block/queue-sysfs.rst b/Documentation/block/queue-sysfs.rst
> > > index 2638d3446b79..cd371a821855 100644
> > > --- a/Documentation/block/queue-sysfs.rst
> > > +++ b/Documentation/block/queue-sysfs.rst
> > > @@ -273,4 +273,11 @@ devices are described in the ZBC (Zoned Block Commands) and ZAC
> > >  do not support zone commands, they will be treated as regular block devices
> > >  and zoned will report "none".
> > >  
> > > +limit_bio_size (RW)
> > > +-------------------
> > > +This indicates QUEUE_FLAG_LIMIT_BIO_SIZE queue flag value. And
> > > +QUEUE_FLAG_LIMIT_BIO_SIZE can be changed via set(1)/clear(0) this node.
> > > +bio max size will be limited by queue max sectors via set this node. And
> > > +limit of bio max size will be cleard via clear this node.
> > > +
> > >  Jens Axboe <jens.axboe@xxxxxxxxxx>, February 2009
> > > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > > index b513f1683af0..840d97f427e6 100644
> > > --- a/block/blk-sysfs.c
> > > +++ b/block/blk-sysfs.c
> > > @@ -288,6 +288,7 @@ QUEUE_SYSFS_BIT_FNS(nonrot, NONROT, 1);
> > >  QUEUE_SYSFS_BIT_FNS(random, ADD_RANDOM, 0);
> > >  QUEUE_SYSFS_BIT_FNS(iostats, IO_STAT, 0);
> > >  QUEUE_SYSFS_BIT_FNS(stable_writes, STABLE_WRITES, 0);
> > > +QUEUE_SYSFS_BIT_FNS(limit_bio_size, LIMIT_BIO_SIZE, 0);
> > >  #undef QUEUE_SYSFS_BIT_FNS
> > >  
> > >  static ssize_t queue_zoned_show(struct request_queue *q, char *page)
> > > @@ -615,6 +616,7 @@ QUEUE_RW_ENTRY(queue_nonrot, "rotational");
> > >  QUEUE_RW_ENTRY(queue_iostats, "iostats");
> > >  QUEUE_RW_ENTRY(queue_random, "add_random");
> > >  QUEUE_RW_ENTRY(queue_stable_writes, "stable_writes");
> > > +QUEUE_RW_ENTRY(queue_limit_bio_size, "limit_bio_size");
> > >  
> > >  static struct attribute *queue_attrs[] = {
> > >  	&queue_requests_entry.attr,
> > > @@ -648,6 +650,7 @@ static struct attribute *queue_attrs[] = {
> > >  	&queue_rq_affinity_entry.attr,
> > >  	&queue_iostats_entry.attr,
> > >  	&queue_stable_writes_entry.attr,
> > > +	&queue_limit_bio_size_entry.attr,
> > >  	&queue_random_entry.attr,
> > >  	&queue_poll_entry.attr,
> > >  	&queue_wc_entry.attr,
> > 
> > Has it been considered to introduce a function to set the BIO size limit
> > instead of introducing a new sysfs attribute? See also
> > blk_queue_max_hw_sectors().
> 
> A function to set has been not considered yet.
> But sysfs attribute should be supported I think. Because it can be
> different depending on each system environment including policy.

But what tool is now responsible for setting this new value?  Where will
it get that information from?  And why can't the kernel just
automatically set this correctly in the first place without any need for
userspace interaction?

thanks,

greg k-h