Re: [PATCH v2 3/7] buffer: Fix grow_buffers() for block size > PAGE_SIZE

[Ryusuke Konishi <konishi.ryusuke@gmail.com>]

On Fri, Nov 10, 2023 at 3:37 PM Ryusuke Konishi wrote:
>
> On Fri, Nov 10, 2023 at 6:06 AM Matthew Wilcox (Oracle) wrote:
> >
> > We must not shift by a negative number so work in terms of a byte
> > offset to avoid the awkward shift left-or-right-depending-on-sign
> > option.  This means we need to use check_mul_overflow() to ensure
> > that a large block number does not result in a wrap.
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  fs/buffer.c | 17 ++++++-----------
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index 44e0c0b7f71f..9c3f49cf8d28 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -1085,26 +1085,21 @@ static bool grow_dev_folio(struct block_device *bdev, sector_t block,
> >  static bool grow_buffers(struct block_device *bdev, sector_t block,
> >                 unsigned size, gfp_t gfp)
> >  {
> > -       pgoff_t index;
> > -       int sizebits;
> > -
> > -       sizebits = PAGE_SHIFT - __ffs(size);
> > -       index = block >> sizebits;
> > +       loff_t pos;
> >
> >         /*
> > -        * Check for a block which wants to lie outside our maximum possible
> > -        * pagecache index.  (this comparison is done using sector_t types).
> > +        * Check for a block which lies outside our maximum possible
> > +        * pagecache index.
> >          */
> > -       if (unlikely(index != block >> sizebits)) {
> > -               printk(KERN_ERR "%s: requested out-of-range block %llu for "
> > -                       "device %pg\n",
> > +       if (check_mul_overflow(block, size, &pos) || pos > MAX_LFS_FILESIZE) {
> > +               printk(KERN_ERR "%s: requested out-of-range block %llu for device %pg\n",
> >                         __func__, (unsigned long long)block,
> >                         bdev);
> >                 return false;
> >         }
> >

> >         /* Create a folio with the proper size buffers */
> > -       return grow_dev_folio(bdev, block, index, size, gfp);
>
> > +       return grow_dev_folio(bdev, block, pos / PAGE_SIZE, size, gfp);
>
> "pos" has a loff_t type (= long long type).
> Was it okay to do C division directly on 32-bit architectures?
>
> Regards,
> Ryusuke Konishi

Similar to the comment for patch 5/7, can we safely use the generally
less expensive shift operation "pos >> PAGE_SHIFT" here ?

Regards,
Ryusuke Konishi