| Message ID | 20240827032218.34744-1-liwang@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | loop: Increase bsize variable from unsigned short to unsigned int | expand |
Adding Jens Axboe and linux-block@vger.kernel.org ML. On Tue, Aug 27, 2024 at 11:22 AM Li Wang <liwang@redhat.com> wrote: > This change allows the loopback driver to handle larger block sizes > and increases the consistency of data types used within the driver. > Especially to mactch the struct queue_limits.logical_block_size type. > > Also, this is to get rid of the LTP/ioctl_loop06 test failure: > > 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > > PAGE_SIZE > 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > ... > 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > > PAGE_SIZE > 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > Link: https://lists.linux.it/pipermail/ltp/2024-August/039912.html > Signed-off-by: Li Wang <liwang@redhat.com> > Cc: Jan Stancek <jstancek@redhat.com> > Cc: John Garry <john.g.garry@oracle.com> > Cc: Damien Le Moal <dlemoal@kernel.org> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > --- > drivers/block/loop.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 78a7bb28defe..86cc3b19faae 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -173,7 +173,7 @@ static loff_t get_loop_size(struct loop_device *lo, > struct file *file) > static bool lo_bdev_can_use_dio(struct loop_device *lo, > struct block_device *backing_bdev) > { > - unsigned short sb_bsize = bdev_logical_block_size(backing_bdev); > + unsigned int sb_bsize = bdev_logical_block_size(backing_bdev); > > if (queue_logical_block_size(lo->lo_queue) < sb_bsize) > return false; > @@ -977,7 +977,7 @@ loop_set_status_from_info(struct loop_device *lo, > return 0; > } > > -static unsigned short loop_default_blocksize(struct loop_device *lo, > +static unsigned int loop_default_blocksize(struct loop_device *lo, > struct block_device *backing_bdev) > { > /* In case of direct I/O, match underlying block size */ > @@ -986,7 +986,7 @@ static unsigned short loop_default_blocksize(struct > loop_device *lo, > return SECTOR_SIZE; > } > > -static int loop_reconfigure_limits(struct loop_device *lo, unsigned short > bsize) > +static int loop_reconfigure_limits(struct loop_device *lo, unsigned int > bsize) > { > struct file *file = lo->lo_backing_file; > struct inode *inode = file->f_mapping->host; > -- > 2.46.0 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > >
On 8/27/24 12:22, Li Wang wrote: > This change allows the loopback driver to handle larger block sizes > and increases the consistency of data types used within the driver. > Especially to mactch the struct queue_limits.logical_block_size type. > > Also, this is to get rid of the LTP/ioctl_loop06 test failure: > > 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > PAGE_SIZE > 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > ... > 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > PAGE_SIZE > 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > Link: https://lists.linux.it/pipermail/ltp/2024-August/039912.html > Signed-off-by: Li Wang <liwang@redhat.com> > Cc: Jan Stancek <jstancek@redhat.com> > Cc: John Garry <john.g.garry@oracle.com> > Cc: Damien Le Moal <dlemoal@kernel.org> > Cc: Stefan Hajnoczi <stefanha@redhat.com> Looks OK to me. Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
On Tue, Aug 27, 2024 at 5:22 AM Li Wang <liwang@redhat.com> wrote: > > This change allows the loopback driver to handle larger block sizes > and increases the consistency of data types used within the driver. > Especially to mactch the struct queue_limits.logical_block_size type. ^^ small typo here, extra 'c' in "match" > > Also, this is to get rid of the LTP/ioctl_loop06 test failure: > > 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > PAGE_SIZE > 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > ... > 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > PAGE_SIZE > 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > Link: https://lists.linux.it/pipermail/ltp/2024-August/039912.html > Signed-off-by: Li Wang <liwang@redhat.com> > Cc: Jan Stancek <jstancek@redhat.com> > Cc: John Garry <john.g.garry@oracle.com> > Cc: Damien Le Moal <dlemoal@kernel.org> > Cc: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Jan Stancek <jstancek@redhat.com>
On 27/08/2024 04:22, Li Wang wrote: +linux-block, Jens > This change allows the loopback driver to handle larger block sizes larger than what? PAGE_SIZE? > and increases the consistency of data types used within the driver. > Especially to mactch the struct queue_limits.logical_block_size type. > > Also, this is to get rid of the LTP/ioctl_loop06 test failure: > > 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > PAGE_SIZE > 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > ... > 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > PAGE_SIZE > 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > Link: https://urldefense.com/v3/__https://lists.linux.it/pipermail/ltp/2024-August/039912.html__;!!ACWV5N9M2RV99hQ!Kxyf0QaP903VtqbEb5n4dgWFhDjWex6vfZhJ9i2ewSqvAWf_iqFNNoOLlJm2BR_ofSSwGwowUQbky65jbg$ > Signed-off-by: Li Wang <liwang@redhat.com> > Cc: Jan Stancek <jstancek@redhat.com> > Cc: John Garry <john.g.garry@oracle.com> > Cc: Damien Le Moal <dlemoal@kernel.org> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > --- > drivers/block/loop.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 78a7bb28defe..86cc3b19faae 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -173,7 +173,7 @@ static loff_t get_loop_size(struct loop_device *lo, struct file *file) > static bool lo_bdev_can_use_dio(struct loop_device *lo, > struct block_device *backing_bdev) > { > - unsigned short sb_bsize = bdev_logical_block_size(backing_bdev); > + unsigned int sb_bsize = bdev_logical_block_size(backing_bdev); > > if (queue_logical_block_size(lo->lo_queue) < sb_bsize) > return false; > @@ -977,7 +977,7 @@ loop_set_status_from_info(struct loop_device *lo, > return 0; > } > > -static unsigned short loop_default_blocksize(struct loop_device *lo, > +static unsigned int loop_default_blocksize(struct loop_device *lo, > struct block_device *backing_bdev) > { > /* In case of direct I/O, match underlying block size */ > @@ -986,7 +986,7 @@ static unsigned short loop_default_blocksize(struct loop_device *lo, > return SECTOR_SIZE; > } > > -static int loop_reconfigure_limits(struct loop_device *lo, unsigned short bsize) > +static int loop_reconfigure_limits(struct loop_device *lo, unsigned int bsize) > { > struct file *file = lo->lo_backing_file; > struct inode *inode = file->f_mapping->host;
On Tue, Aug 27, 2024 at 3:20 PM John Garry <john.g.garry@oracle.com> wrote: > > On 27/08/2024 04:22, Li Wang wrote: > > +linux-block, Jens > > > This change allows the loopback driver to handle larger block sizes > > larger than what? PAGE_SIZE? Yes, system should return EINVAL when the tested bsize is larger than PAGE_SIZE. But due to the loop_reconfigure_limits() cast it to 'unsined short' that never gets an expected error when testing invalid logical block size. That's why LTP/ioctl_loop06 failed on a system with 64k (ppc64le) pagesize (since kernel-v6.11-rc1 with patches): 9423c653fe6110 ("loop: Don't bother validating blocksize") fe3d508ba95bc6 ("block: Validate logical block size in blk_validate_limits()") -- Regards, Li Wang
On 27/08/2024 08:35, Li Wang wrote: > On Tue, Aug 27, 2024 at 3:20 PM John Garry <john.g.garry@oracle.com> wrote: >> >> On 27/08/2024 04:22, Li Wang wrote: >> >> +linux-block, Jens >> >>> This change allows the loopback driver to handle larger block sizes >> >> larger than what? PAGE_SIZE? > > Yes, Please then explicitly mention that > system should return EINVAL when the tested bsize is larger than PAGE_SIZE. > But due to the loop_reconfigure_limits() cast it to 'unsined short' that > never gets an expected error when testing invalid logical block size.> > That's why LTP/ioctl_loop06 failed on a system with 64k (ppc64le) pagesize > (since kernel-v6.11-rc1 with patches): > > 9423c653fe6110 ("loop: Don't bother validating blocksize") > fe3d508ba95bc6 ("block: Validate logical block size in blk_validate_limits()") > > > I feel that you should be adding a fixes tag, but it seems that those commits only expose the issue, and whatever introduced unsigned short usage was not right. Maybe you can just get this included for 6.11, where 9423c653fe6110 was introduced. Thanks, John
John Garry <john.g.garry@oracle.com> wrote: > On 27/08/2024 08:35, Li Wang wrote: > > On Tue, Aug 27, 2024 at 3:20 PM John Garry <john.g.garry@oracle.com> wrote: > >> > >> On 27/08/2024 04:22, Li Wang wrote: > >> > >> +linux-block, Jens > >> > >>> This change allows the loopback driver to handle larger block sizes > >> > >> larger than what? PAGE_SIZE? > > > > Yes, > > Please then explicitly mention that Sure. > > > system should return EINVAL when the tested bsize is larger than PAGE_SIZE. > > But due to the loop_reconfigure_limits() cast it to 'unsined short' that > > never gets an expected error when testing invalid logical block size.> > > That's why LTP/ioctl_loop06 failed on a system with 64k (ppc64le) pagesize > > (since kernel-v6.11-rc1 with patches): > > > > 9423c653fe6110 ("loop: Don't bother validating blocksize") > > fe3d508ba95bc6 ("block: Validate logical block size in blk_validate_limits()") > > > > > > > > I feel that you should be adding a fixes tag, but it seems that those > commits only expose the issue, and whatever introduced unsigned short > usage was not right. Maybe you can just get this included for 6.11, > where 9423c653fe6110 was introduced. Ok, we can mention that two commits exposed the problem since v6.11-rc1. I will send V2 including that. Thanks!
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 78a7bb28defe..86cc3b19faae 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -173,7 +173,7 @@ static loff_t get_loop_size(struct loop_device *lo, struct file *file) static bool lo_bdev_can_use_dio(struct loop_device *lo, struct block_device *backing_bdev) { - unsigned short sb_bsize = bdev_logical_block_size(backing_bdev); + unsigned int sb_bsize = bdev_logical_block_size(backing_bdev); if (queue_logical_block_size(lo->lo_queue) < sb_bsize) return false; @@ -977,7 +977,7 @@ loop_set_status_from_info(struct loop_device *lo, return 0; } -static unsigned short loop_default_blocksize(struct loop_device *lo, +static unsigned int loop_default_blocksize(struct loop_device *lo, struct block_device *backing_bdev) { /* In case of direct I/O, match underlying block size */ @@ -986,7 +986,7 @@ static unsigned short loop_default_blocksize(struct loop_device *lo, return SECTOR_SIZE; } -static int loop_reconfigure_limits(struct loop_device *lo, unsigned short bsize) +static int loop_reconfigure_limits(struct loop_device *lo, unsigned int bsize) { struct file *file = lo->lo_backing_file; struct inode *inode = file->f_mapping->host;
This change allows the loopback driver to handle larger block sizes and increases the consistency of data types used within the driver. Especially to mactch the struct queue_limits.logical_block_size type. Also, this is to get rid of the LTP/ioctl_loop06 test failure: 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > PAGE_SIZE 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly ... 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > PAGE_SIZE 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly Link: https://lists.linux.it/pipermail/ltp/2024-August/039912.html Signed-off-by: Li Wang <liwang@redhat.com> Cc: Jan Stancek <jstancek@redhat.com> Cc: John Garry <john.g.garry@oracle.com> Cc: Damien Le Moal <dlemoal@kernel.org> Cc: Stefan Hajnoczi <stefanha@redhat.com> --- drivers/block/loop.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)