On Tue, Nov 04, 2025 at 03:12:53AM -0800, Christoph Hellwig wrote:
> On Mon, Nov 03, 2025 at 08:48:29AM -0800, Eric Biggers wrote:
> > >   *inode_ret = inode;
> > > - *lblk_num_ret = ((u64)folio->index << (PAGE_SHIFT - inode->i_blkbits)) +
> > > + *lblk_num_ret = (((u64)folio->index << PAGE_SHIFT) >> inode->i_blkbits) 
> > > +
> 
> This should be using folio_pos() instead of open coding the arithmetics.

Well, folio_pos() doesn't work with sizes greater than S64_MAX, and it
uses multiplication rather than a shift.

Probably doesn't matter, but I always feel like I have to actually check
that.

It looks like the size of block device can come from several different
places, including set_capacity(), bdev_resize_partition(), and
add_partition().  The first has a size check.  I don't immediately see a
size check in the other two.  Maybe it's there and I need to look
closer.  Also can the size of a block device be set in other ways?

Then I have to remember whether a multiplication of a signed value gets
reliably optimized to a shift on all architectures or not.  I think so.

Anyway, the trivial version avoids having to consider any of this...

- Eric

Reply via email to