Re: [PATCH v2 5/7] buffer: Fix various functions for block size > PAGE_SIZE

From: Ryusuke Konishi <konishi.ryusuke@gmail.com>
To: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Hannes Reinecke <hare@suse.de>,
	 Luis Chamberlain <mcgrof@kernel.org>,
	Pankaj Raghav <p.raghav@samsung.com>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 5/7] buffer: Fix various functions for block size > PAGE_SIZE
Date: Fri, 10 Nov 2023 16:48:02 +0900	[thread overview]
Message-ID: <CAKFNMokuZFWqoX_1uWm0-vTcbo_gESkNpv8J8Pw1G-Vwd=-D+w@mail.gmail.com> (raw)
In-Reply-To: <20231109210608.2252323-6-willy@infradead.org>

On Fri, Nov 10, 2023 at 6:06 AM Matthew Wilcox (Oracle) wrote:
>
> If i_blkbits is larger than PAGE_SHIFT, we shift by a negative number,
> which is undefined.  It is safe to shift the block left as a block
> device must be smaller than MAX_LFS_FILESIZE, which is guaranteed to
> fit in loff_t.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>
> ---
>  fs/buffer.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index ab345fd4da12..faf1916200c2 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -199,7 +199,7 @@ __find_get_block_slow(struct block_device *bdev, sector_t block)
>         int all_mapped = 1;
>         static DEFINE_RATELIMIT_STATE(last_warned, HZ, 1);
>
> -       index = block >> (PAGE_SHIFT - bd_inode->i_blkbits);
> +       index = ((loff_t)block << bd_inode->i_blkbits) / PAGE_SIZE;

Multiple 64-bit divisions are used in this patch, but why not use two
stage shifts as shown below if there is no left shift overflow and the
sign bit will not be set ?

       index = ((loff_t)block << bd_inode->i_blkbits) >> PAGE_SHIFT;

Regards,
Ryusuke Konishi

>         folio = __filemap_get_folio(bd_mapping, index, FGP_ACCESSED, 0);
>         if (IS_ERR(folio))
>                 goto out;
> @@ -1693,13 +1693,13 @@ void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len)
>         struct inode *bd_inode = bdev->bd_inode;
>         struct address_space *bd_mapping = bd_inode->i_mapping;
>         struct folio_batch fbatch;
> -       pgoff_t index = block >> (PAGE_SHIFT - bd_inode->i_blkbits);
> +       pgoff_t index = ((loff_t)block << bd_inode->i_blkbits) / PAGE_SIZE;
>         pgoff_t end;
>         int i, count;
>         struct buffer_head *bh;
>         struct buffer_head *head;
>
> -       end = (block + len - 1) >> (PAGE_SHIFT - bd_inode->i_blkbits);
> +       end = ((loff_t)(block + len - 1) << bd_inode->i_blkbits) / PAGE_SIZE;
>         folio_batch_init(&fbatch);
>         while (filemap_get_folios(bd_mapping, &index, end, &fbatch)) {
>                 count = folio_batch_count(&fbatch);
> @@ -2660,8 +2660,8 @@ int block_truncate_page(struct address_space *mapping,
>                 return 0;
>
>         length = blocksize - length;
> -       iblock = (sector_t)index << (PAGE_SHIFT - inode->i_blkbits);
> -
> +       iblock = ((loff_t)index * PAGE_SIZE) >> inode->i_blkbits;
> +
>         folio = filemap_grab_folio(mapping, index);
>         if (IS_ERR(folio))
>                 return PTR_ERR(folio);
> --
> 2.42.0
>
>