loop: take the file system minimum dio alignment into account v2

loop: take the file system minimum dio alignment into account v2

[Christoph Hellwig]

Hi Jens,

this series ensures that the loop device blocksize default also works on
file systems that require a bigger direct I/O alignment than the logical
block size of sb->s_bdev.

Changes since v1:
 - also do the right thing when direct I/O is enabled through loop flag
   and not by passing in a O_DIRECT FD
 - minor cleanups to the calling convention of a helper

Diffstat:
 loop.c |   84 +++++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 46 insertions(+), 38 deletions(-)

[PATCH 1/4] loop: factor out a loop_assign_backing_file helper

[Christoph Hellwig]

Split the code for setting up a backing file into a helper in preparation
of adding more code to this path.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 1ec7417c7f00..85a6aa551bb5 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -573,6 +573,14 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
 	return 0;
 }
 
+static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
+{
+	lo->lo_backing_file = file;
+	lo->old_gfp_mask = mapping_gfp_mask(file->f_mapping);
+	mapping_set_gfp_mask(file->f_mapping,
+			lo->old_gfp_mask & ~(__GFP_IO | __GFP_FS));
+}
+
 /*
  * loop_change_fd switched the backing store of a loopback device to
  * a new file. This is useful for operating system installers to free up
@@ -625,10 +633,7 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	disk_force_media_change(lo->lo_disk);
 	blk_mq_freeze_queue(lo->lo_queue);
 	mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
-	lo->lo_backing_file = file;
-	lo->old_gfp_mask = mapping_gfp_mask(file->f_mapping);
-	mapping_set_gfp_mask(file->f_mapping,
-			     lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
+	loop_assign_backing_file(lo, file);
 	loop_update_dio(lo);
 	blk_mq_unfreeze_queue(lo->lo_queue);
 	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
@@ -1018,7 +1023,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 			  const struct loop_config *config)
 {
 	struct file *file = fget(config->fd);
-	struct address_space *mapping;
 	struct queue_limits lim;
 	int error;
 	loff_t size;
@@ -1054,8 +1058,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 	if (error)
 		goto out_unlock;
 
-	mapping = file->f_mapping;
-
 	if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0) {
 		error = -EINVAL;
 		goto out_unlock;
@@ -1087,9 +1089,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 	set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0);
 
 	lo->lo_device = bdev;
-	lo->lo_backing_file = file;
-	lo->old_gfp_mask = mapping_gfp_mask(mapping);
-	mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
+	loop_assign_backing_file(lo, file);
 
 	lim = queue_limits_start_update(lo->lo_queue);
 	loop_update_limits(lo, &lim, config->block_size);
-- 
2.45.2

Re: [PATCH 1/4] loop: factor out a loop_assign_backing_file helper

[Damien Le Moal]

On 1/31/25 21:00, Christoph Hellwig wrote:
> Split the code for setting up a backing file into a helper in preparation
> of adding more code to this path.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/4] loop: set LO_FLAGS_DIRECT_IO in loop_assign_backing_file

[Christoph Hellwig]

Assigning LO_FLAGS_DIRECT_IO from the O_DIRECT flag is related to
assigning a new backing file.  Move the assignment in preparation
of using the flag more and earlier.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 85a6aa551bb5..11f483d43bf4 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -211,8 +211,6 @@ static inline void loop_update_dio(struct loop_device *lo)
 	WARN_ON_ONCE(lo->lo_state == Lo_bound &&
 		     lo->lo_queue->mq_freeze_depth == 0);
 
-	if (lo->lo_backing_file->f_flags & O_DIRECT)
-		lo->lo_flags |= LO_FLAGS_DIRECT_IO;
 	if ((lo->lo_flags & LO_FLAGS_DIRECT_IO) && !lo_can_use_dio(lo))
 		lo->lo_flags &= ~LO_FLAGS_DIRECT_IO;
 
@@ -579,6 +577,8 @@ static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
 	lo->old_gfp_mask = mapping_gfp_mask(file->f_mapping);
 	mapping_set_gfp_mask(file->f_mapping,
 			lo->old_gfp_mask & ~(__GFP_IO | __GFP_FS));
+	if (lo->lo_backing_file->f_flags & O_DIRECT)
+		lo->lo_flags |= LO_FLAGS_DIRECT_IO;
 }
 
 /*
-- 
2.45.2

Re: [PATCH 2/4] loop: set LO_FLAGS_DIRECT_IO in loop_assign_backing_file

[Damien Le Moal]

On 1/31/25 21:00, Christoph Hellwig wrote:
> Assigning LO_FLAGS_DIRECT_IO from the O_DIRECT flag is related to
> assigning a new backing file.  Move the assignment in preparation
> of using the flag more and earlier.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 3/4] loop: check in LO_FLAGS_DIRECT_IO in loop_default_blocksize

[Christoph Hellwig]

We can't go below the minimum direct I/O size no matter if direct I/O is
enabled by passing in an O_DIRECT file descriptor or due to the explicit
flag.  Now that LO_FLAGS_DIRECT_IO is set earlier after assigning a
backing file, loop_default_blocksize can check it instead of the
O_DIRECT flag to handle both conditions.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 11f483d43bf4..36b01c36e06b 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -979,7 +979,7 @@ static unsigned int loop_default_blocksize(struct loop_device *lo,
 		struct block_device *backing_bdev)
 {
 	/* In case of direct I/O, match underlying block size */
-	if ((lo->lo_backing_file->f_flags & O_DIRECT) && backing_bdev)
+	if ((lo->lo_flags & LO_FLAGS_DIRECT_IO) && backing_bdev)
 		return bdev_logical_block_size(backing_bdev);
 	return SECTOR_SIZE;
 }
-- 
2.45.2

Re: [PATCH 3/4] loop: check in LO_FLAGS_DIRECT_IO in loop_default_blocksize

[Damien Le Moal]

On 1/31/25 21:00, Christoph Hellwig wrote:
> We can't go below the minimum direct I/O size no matter if direct I/O is
> enabled by passing in an O_DIRECT file descriptor or due to the explicit
> flag.  Now that LO_FLAGS_DIRECT_IO is set earlier after assigning a
> backing file, loop_default_blocksize can check it instead of the
> O_DIRECT flag to handle both conditions.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 4/4] loop: take the file system minimum dio alignment into account

[Christoph Hellwig]

The loop driver currently uses the logical block size of the underlying
bdev as the lower bound of the loop device block size.  While this works
for many cases, it fails for file systems made up of multiple devices
with different logic block size (e.g. XFS with a RT device that has a
larger logical block size), or when the file systems doesn't support
direct I/O writes at the sector size granularity (e.g. because it does
out of place writes with a file system block size larger than the sector
size).

Fix this by querying the minimum direct I/O alignment from statx when
available.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 60 +++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 36b01c36e06b..89352a85b704 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -54,7 +54,8 @@ struct loop_device {
 	int		lo_flags;
 	char		lo_file_name[LO_NAME_SIZE];
 
-	struct file *	lo_backing_file;
+	struct file	*lo_backing_file;
+	unsigned int	lo_min_dio_size;
 	struct block_device *lo_device;
 
 	gfp_t		old_gfp_mask;
@@ -169,29 +170,14 @@ static loff_t get_loop_size(struct loop_device *lo, struct file *file)
  * of backing device, and the logical block size of loop is bigger than that of
  * the backing device.
  */
-static bool lo_bdev_can_use_dio(struct loop_device *lo,
-		struct block_device *backing_bdev)
-{
-	unsigned int sb_bsize = bdev_logical_block_size(backing_bdev);
-
-	if (queue_logical_block_size(lo->lo_queue) < sb_bsize)
-		return false;
-	if (lo->lo_offset & (sb_bsize - 1))
-		return false;
-	return true;
-}
-
 static bool lo_can_use_dio(struct loop_device *lo)
 {
-	struct inode *inode = lo->lo_backing_file->f_mapping->host;
-
 	if (!(lo->lo_backing_file->f_mode & FMODE_CAN_ODIRECT))
 		return false;
-
-	if (S_ISBLK(inode->i_mode))
-		return lo_bdev_can_use_dio(lo, I_BDEV(inode));
-	if (inode->i_sb->s_bdev)
-		return lo_bdev_can_use_dio(lo, inode->i_sb->s_bdev);
+	if (queue_logical_block_size(lo->lo_queue) < lo->lo_min_dio_size)
+		return false;
+	if (lo->lo_offset & (lo->lo_min_dio_size - 1))
+		return false;
 	return true;
 }
 
@@ -539,6 +525,28 @@ static void loop_reread_partitions(struct loop_device *lo)
 			__func__, lo->lo_number, lo->lo_file_name, rc);
 }
 
+static unsigned int loop_query_min_dio_size(struct loop_device *lo)
+{
+	struct file *file = lo->lo_backing_file;
+	struct block_device *sb_bdev = file->f_mapping->host->i_sb->s_bdev;
+	struct kstat st;
+
+	/*
+	 * Use the minimal dio alignment of the file system if provided.
+	 */
+	if (!vfs_getattr(&file->f_path, &st, STATX_DIOALIGN, 0) &&
+	    (st.result_mask & STATX_DIOALIGN))
+		return st.dio_offset_align;
+
+	/*
+	 * In a perfect world this wouldn't be needed, but as of Linux 6.13 only
+	 * a handful of file systems support the STATX_DIOALIGN flag.
+	 */
+	if (sb_bdev)
+		return bdev_logical_block_size(sb_bdev);
+	return SECTOR_SIZE;
+}
+
 static inline int is_loop_device(struct file *file)
 {
 	struct inode *i = file->f_mapping->host;
@@ -579,6 +587,7 @@ static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
 			lo->old_gfp_mask & ~(__GFP_IO | __GFP_FS));
 	if (lo->lo_backing_file->f_flags & O_DIRECT)
 		lo->lo_flags |= LO_FLAGS_DIRECT_IO;
+	lo->lo_min_dio_size = loop_query_min_dio_size(lo);
 }
 
 /*
@@ -975,12 +984,11 @@ loop_set_status_from_info(struct loop_device *lo,
 	return 0;
 }
 
-static unsigned int loop_default_blocksize(struct loop_device *lo,
-		struct block_device *backing_bdev)
+static unsigned int loop_default_blocksize(struct loop_device *lo)
 {
-	/* In case of direct I/O, match underlying block size */
-	if ((lo->lo_flags & LO_FLAGS_DIRECT_IO) && backing_bdev)
-		return bdev_logical_block_size(backing_bdev);
+	/* In case of direct I/O, match underlying minimum I/O size */
+	if (lo->lo_flags & LO_FLAGS_DIRECT_IO)
+		return lo->lo_min_dio_size;
 	return SECTOR_SIZE;
 }
 
@@ -998,7 +1006,7 @@ static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim,
 		backing_bdev = inode->i_sb->s_bdev;
 
 	if (!bsize)
-		bsize = loop_default_blocksize(lo, backing_bdev);
+		bsize = loop_default_blocksize(lo);
 
 	loop_get_discard_config(lo, &granularity, &max_discard_sectors);
 
-- 
2.45.2

Re: [PATCH 4/4] loop: take the file system minimum dio alignment into account

[Damien Le Moal]

On 1/31/25 21:00, Christoph Hellwig wrote:
> The loop driver currently uses the logical block size of the underlying
> bdev as the lower bound of the loop device block size.  While this works
> for many cases, it fails for file systems made up of multiple devices
> with different logic block size (e.g. XFS with a RT device that has a

Nit: s/logic block size/logical block sizes

> larger logical block size), or when the file systems doesn't support
> direct I/O writes at the sector size granularity (e.g. because it does
> out of place writes with a file system block size larger than the sector
> size).
> 
> Fix this by querying the minimum direct I/O alignment from statx when
> available.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Other than that, looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: loop: take the file system minimum dio alignment into account v2

[Christoph Hellwig]

ping?

Re: loop: take the file system minimum dio alignment into account v2

[Jens Axboe]

On Fri, 31 Jan 2025 13:00:37 +0100, Christoph Hellwig wrote:
> this series ensures that the loop device blocksize default also works on
> file systems that require a bigger direct I/O alignment than the logical
> block size of sb->s_bdev.
> 
> Changes since v1:
>  - also do the right thing when direct I/O is enabled through loop flag
>    and not by passing in a O_DIRECT FD
>  - minor cleanups to the calling convention of a helper
> 
> [...]

Applied, thanks!

[1/4] loop: factor out a loop_assign_backing_file helper
      commit: d278164832618bf2775c6a89e6434e2633de1eed
[2/4] loop: set LO_FLAGS_DIRECT_IO in loop_assign_backing_file
      commit: 984c2ab4b87c0db7c53c3b6a42be95f79f2aae89
[3/4] loop: check in LO_FLAGS_DIRECT_IO in loop_default_blocksize
      commit: f6f9e32fe1e454ae8ac0190b2c2bd6074914beec
[4/4] loop: take the file system minimum dio alignment into account
      commit: f4774e92aab85d9bb5c76463f220ad7ba535bb1c

Best regards,
-- 
Jens Axboe