Re: [PATCH v11 02/16] xfs: only call xfs_setsize_buftarg once per buffer target

John Garry
2025-05-05T15:28:26Z

On 05/05/2025 15:48, John Garry wrote:
>>> @Darrick, please comment on whether happy with changes discussed.
>> I put the sync_blockdev calls in a separate function so that the
>> EIO/ENOSPC/whatever errors that come from the block device sync don't
>> get morphed into ENOMEM by xfs_alloc_buftarg before being passed up.  I
>> suppose we could make that function return an ERR_PTR, but I was trying
>> to avoid making even more changes at the last minute, again.
>
> It seems simpler to just have the individual sync_blockdev() calls from
> xfs_alloc_buftarg(), rather than adding ERR_PTR() et al handling in both
> xfs_alloc_buftarg() and xfs_open_devices().

Which of the following is better:

------8<-----
int
@@ -1810,10 +1806,10 @@ xfs_alloc_buftarg(
* When allocating the buftargs we have not yet read the super block and
* thus don't know the file system sector size yet.
*/
- if (xfs_setsize_buftarg(btp, bdev_logical_block_size(btp->bt_bdev)))
- goto error_free;
- if (xfs_init_buftarg(btp, bdev_logical_block_size(btp->bt_bdev),
- mp->m_super->s_id))
+ btp->bt_meta_sectorsize = bdev_logical_block_size(btp->bt_bdev);
+ btp->bt_meta_sectormask = btp->bt_meta_sectorsize - 1;
+
+ if (xfs_init_buftarg(btp, btp->bt_meta_sectorsize, mp->m_super->s_id))
goto error_free;

return btp;
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 5e456a6073ca..48d5b630fe46 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -481,21 +481,38 @@ xfs_open_devices(

/*
* Setup xfs_mount buffer target pointers
+ *
+ * Flush and invalidate all devices' pagecaches before reading any
+ * metadata because XFS doesn't use the bdev pagecache.
*/
- error = -ENOMEM;
mp->m_ddev_targp = xfs_alloc_buftarg(mp, sb->s_bdev_file);
- if (!mp->m_ddev_targp)
+ if (!mp->m_ddev_targp) {
+ error = -ENOMEM;
+ goto out_close_rtdev;
+ }
+ error = sync_blockdev(mp->m_ddev_targp->bt_bdev);
+ if (error)
goto out_close_rtdev;

if (rtdev_file) {
mp->m_rtdev_targp = xfs_alloc_buftarg(mp, rtdev_file);
- if (!mp->m_rtdev_targp)
+ if (!mp->m_rtdev_targp) {
+ error = -ENOMEM;
+ goto out_free_ddev_targ;
+ }
+ error = sync_blockdev(mp->m_rtdev_targp->bt_bdev);
+ if (error)
goto out_free_ddev_targ;
}

if (logdev_file && file_bdev(logdev_file) != ddev) {
mp->m_logdev_targp = xfs_alloc_buftarg(mp, logdev_file);
- if (!mp->m_logdev_targp)
+ if (!mp->m_logdev_targp) {
+ error = -ENOMEM;
+ goto out_free_rtdev_targ;
+ }
+ error = sync_blockdev(mp->m_logdev_targp->bt_bdev);
+ if (error)
goto out_free_rtdev_targ;

----->8------

int
@@ -1786,6 +1782,8 @@ xfs_alloc_buftarg(
{
struct xfs_buftarg *btp;
const struct dax_holder_operations *ops = NULL;
+ int error;
+

#if defined(CONFIG_FS_DAX) && defined(CONFIG_MEMORY_FAILURE)
ops = &xfs_dax_holder_operations;
@@ -1806,21 +1804,31 @@ xfs_alloc_buftarg(
btp->bt_bdev);
}

+ /*
+ * Flush and invalidate all devices' pagecaches before reading any
+ * metadata because XFS doesn't use the bdev pagecache.
+ */
+ error = sync_blockdev(mp->m_ddev_targp->bt_bdev);
+ if (error)
+ goto error_free;
+
/*
* When allocating the buftargs we have not yet read the super block and
* thus don't know the file system sector size yet.
*/
- if (xfs_setsize_buftarg(btp, bdev_logical_block_size(btp->bt_bdev)))
- goto error_free;
- if (xfs_init_buftarg(btp, bdev_logical_block_size(btp->bt_bdev),
- mp->m_super->s_id))
+ btp->bt_meta_sectorsize = bdev_logical_block_size(btp->bt_bdev);
+ btp->bt_meta_sectormask = btp->bt_meta_sectorsize - 1;
+
+ if (xfs_init_buftarg(btp, btp->bt_meta_sectorsize, mp->m_super->s_id)) {
+ error = -ENOMEM;
goto error_free;
+ }

return btp;

error_free:
kfree(btp);
- return NULL;
+ return ERR_PTR(error);
}

static inline void
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 5e456a6073ca..4daf0cc480af 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -482,21 +482,26 @@ xfs_open_devices(
/*
* Setup xfs_mount buffer target pointers
*/
- error = -ENOMEM;
mp->m_ddev_targp = xfs_alloc_buftarg(mp, sb->s_bdev_file);
- if (!mp->m_ddev_targp)
+ if (IS_ERR(mp->m_ddev_targp)) {
+ error = PTR_ERR(mp->m_ddev_targp);
goto out_close_rtdev;
+ }

if (rtdev_file) {
mp->m_rtdev_targp = xfs_alloc_buftarg(mp, rtdev_file);
- if (!mp->m_rtdev_targp)
+ if (IS_ERR(mp->m_rtdev_targp)) {
+ error = PTR_ERR(mp->m_rtdev_targp);
goto out_free_ddev_targ;
+ }
}

if (logdev_file && file_bdev(logdev_file) != ddev) {
mp->m_logdev_targp = xfs_alloc_buftarg(mp, logdev_file);
- if (!mp->m_logdev_targp)
+ if (IS_ERR(mp->m_logdev_targp)) {
+ error = PTR_ERR(mp->m_logdev_targp);
goto out_free_rtdev_targ;
+ }

----8<-----