On Tue, Apr 15, 2025 at 09:46:09PM -0700, Christoph Hellwig wrote:
> On Mon, Apr 14, 2025 at 05:33:08PM -0700, Darrick J. Wong wrote:
> > +/*
> > + * For bdev filesystems that do not use buffer heads, check that this block
> > + * size is acceptable and flush dirty pagecache to disk.
> > + */
>
> Can you turn this into a full fledged kerneldoc comment?
Ok.
> > +int bdev_use_blocksize(struct file *file, int size)
> > +{
> > + struct inode *inode = file->f_mapping->host;
> > + struct block_device *bdev = I_BDEV(inode);
> > +
> > + if (blk_validate_block_size(size))
> > + return -EINVAL;
> > +
> > + /* Size cannot be smaller than the size supported by the device */
> > + if (size < bdev_logical_block_size(bdev))
> > + return -EINVAL;
> > +
> > + if (!file->private_data)
> > + return -EINVAL;
>
> This private_data check looks really confusing. Looking it up I see
> that it is directly copied from set_blocksize, but it could really
> use a comment. Or in fact be removed here and kept in set_blocksize
> only as we don't care about an exclusive opener at all. Even there
> a comment would be rather helpful, though.
When even is it null? I thought it would either be the holder or
bdev_inode if not.
> > +
> > + return sync_blockdev(bdev);
> > +}
>
> I don't think we need sync_blockdev here as we don't touch the
> bdev page cache. Maybe XFS wants to still call it, but it feels
> wrong in a helper just validating the block size.
>
> So maybe drop it, rename the helper to bdev_validate_block_size
> and use it in set_blocksize instead of duplicating the logic?
Ok. bdev_validate_block_size is a much better name for a tighter
function...
> > + error = bdev_use_blocksize(btp->bt_bdev_file, sectorsize);
>
> .. and then split using it in XFS into a separate patch from adding
> the block layer helper.
...and xfs can call sync_blockdev directly from xfs_setsize_buftarg.
I imagine we still want any dirty pagecache to get flushed before we
start submitting our own read bios.
--D
