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

[John Garry <john.g.garry@oracle.com>]

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<-----