All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] zonefs fixes
@ 2022-06-03 11:49 Damien Le Moal
  2022-06-03 11:49 ` [PATCH 1/3] zonefs: fix handling of explicit_open option on mount Damien Le Moal
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Damien Le Moal @ 2022-06-03 11:49 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Johannes Thumshirn

3 patches to address zonefs fixes for bugs discovered with an improved
zonefs test suite.

The first 2 patches fix handling of the explicit-open mount option. The
third patch fixes a hang triggered by readahead reaching the end of a
sequential file.

Damien Le Moal (3):
  zonefs: fix handling of explicit_open option on mount
  zonefs: Do not ignore explicit_open with active zone limit
  zonefs: fix zonefs_iomap_begin() for reads

 fs/zonefs/super.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

-- 
2.36.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] zonefs: fix handling of explicit_open option on mount
  2022-06-03 11:49 [PATCH 0/3] zonefs fixes Damien Le Moal
@ 2022-06-03 11:49 ` Damien Le Moal
  2022-06-07  6:01   ` Christoph Hellwig
  2022-06-07  8:46   ` Johannes Thumshirn
  2022-06-03 11:49 ` [PATCH 2/3] zonefs: Do not ignore explicit_open with active zone limit Damien Le Moal
  2022-06-03 11:49 ` [PATCH 3/3] zonefs: fix zonefs_iomap_begin() for reads Damien Le Moal
  2 siblings, 2 replies; 11+ messages in thread
From: Damien Le Moal @ 2022-06-03 11:49 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Johannes Thumshirn

Ignoring the explicit_open mount option on mount for devices that do not
have a limit on the number of open zones must be done after the mount
options are parsed and set in s_mount_opts. Move the check to ignore
the explicit_open option after the call to zonefs_parse_options() in
zonefs_fill_super().

Fixes: b5c00e975779 ("zonefs: open/close zone on file open/close")
Cc: <stable@vger.kernel.org>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 fs/zonefs/super.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index bcb21aea990a..ecce84909ca1 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -1760,12 +1760,6 @@ static int zonefs_fill_super(struct super_block *sb, void *data, int silent)
 
 	atomic_set(&sbi->s_wro_seq_files, 0);
 	sbi->s_max_wro_seq_files = bdev_max_open_zones(sb->s_bdev);
-	if (!sbi->s_max_wro_seq_files &&
-	    sbi->s_mount_opts & ZONEFS_MNTOPT_EXPLICIT_OPEN) {
-		zonefs_info(sb, "No open zones limit. Ignoring explicit_open mount option\n");
-		sbi->s_mount_opts &= ~ZONEFS_MNTOPT_EXPLICIT_OPEN;
-	}
-
 	atomic_set(&sbi->s_active_seq_files, 0);
 	sbi->s_max_active_seq_files = bdev_max_active_zones(sb->s_bdev);
 
@@ -1790,6 +1784,12 @@ static int zonefs_fill_super(struct super_block *sb, void *data, int silent)
 	zonefs_info(sb, "Mounting %u zones",
 		    blkdev_nr_zones(sb->s_bdev->bd_disk));
 
+	if (!sbi->s_max_wro_seq_files &&
+	    sbi->s_mount_opts & ZONEFS_MNTOPT_EXPLICIT_OPEN) {
+		zonefs_info(sb, "No open zones limit. Ignoring explicit_open mount option\n");
+		sbi->s_mount_opts &= ~ZONEFS_MNTOPT_EXPLICIT_OPEN;
+	}
+
 	/* Create root directory inode */
 	ret = -ENOMEM;
 	inode = new_inode(sb);
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] zonefs: Do not ignore explicit_open with active zone limit
  2022-06-03 11:49 [PATCH 0/3] zonefs fixes Damien Le Moal
  2022-06-03 11:49 ` [PATCH 1/3] zonefs: fix handling of explicit_open option on mount Damien Le Moal
@ 2022-06-03 11:49 ` Damien Le Moal
  2022-06-07  6:02   ` Christoph Hellwig
  2022-06-03 11:49 ` [PATCH 3/3] zonefs: fix zonefs_iomap_begin() for reads Damien Le Moal
  2 siblings, 1 reply; 11+ messages in thread
From: Damien Le Moal @ 2022-06-03 11:49 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Johannes Thumshirn

A zoned device may have no limit on the number of open zones but may
have a limit on the number of active zones it can support. In such
case, the explicit_open mount option should not be ignored to ensure
that the open() system call activates the zone with an explicit zone
open command, thus guaranteeing that the zone can be written.

Enforce this by ignoring the explicit_open mount option only for
devices that have both the open and active zone limits equal to 0.

Fixes: 87c9ce3ffec9 ("zonefs: Add active seq file accounting")
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 fs/zonefs/super.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index ecce84909ca1..123464d2145a 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -1085,7 +1085,8 @@ static int zonefs_seq_file_write_open(struct inode *inode)
 
 		if (sbi->s_mount_opts & ZONEFS_MNTOPT_EXPLICIT_OPEN) {
 
-			if (wro > sbi->s_max_wro_seq_files) {
+			if (sbi->s_max_wro_seq_files
+			    && wro > sbi->s_max_wro_seq_files) {
 				atomic_dec(&sbi->s_wro_seq_files);
 				ret = -EBUSY;
 				goto unlock;
@@ -1785,8 +1786,10 @@ static int zonefs_fill_super(struct super_block *sb, void *data, int silent)
 		    blkdev_nr_zones(sb->s_bdev->bd_disk));
 
 	if (!sbi->s_max_wro_seq_files &&
+	    !sbi->s_max_active_seq_files &&
 	    sbi->s_mount_opts & ZONEFS_MNTOPT_EXPLICIT_OPEN) {
-		zonefs_info(sb, "No open zones limit. Ignoring explicit_open mount option\n");
+		zonefs_info(sb,
+			"No open and active zone limits. Ignoring explicit_open mount option\n");
 		sbi->s_mount_opts &= ~ZONEFS_MNTOPT_EXPLICIT_OPEN;
 	}
 
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] zonefs: fix zonefs_iomap_begin() for reads
  2022-06-03 11:49 [PATCH 0/3] zonefs fixes Damien Le Moal
  2022-06-03 11:49 ` [PATCH 1/3] zonefs: fix handling of explicit_open option on mount Damien Le Moal
  2022-06-03 11:49 ` [PATCH 2/3] zonefs: Do not ignore explicit_open with active zone limit Damien Le Moal
@ 2022-06-03 11:49 ` Damien Le Moal
  2022-06-07  6:09   ` Christoph Hellwig
  2 siblings, 1 reply; 11+ messages in thread
From: Damien Le Moal @ 2022-06-03 11:49 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Johannes Thumshirn

If a read operation (e.g. a readahead) is issued to a sequential zone
file with an offset exactly equal to the current file size, the iomap
type will be set to IOMAP_UNWRITTEN, which will prevent an IO, but the
iomap length is always calculated as 0. This causes a WARN_ON() in
iomap_iter():

[17309.548939] WARNING: CPU: 3 PID: 2137 at fs/iomap/iter.c:34 iomap_iter+0x9cf/0xe80
[...]
[17309.650907] RIP: 0010:iomap_iter+0x9cf/0xe80
[...]
[17309.754560] Call Trace:
[17309.757078]  <TASK>
[17309.759240]  ? lock_is_held_type+0xd8/0x130
[17309.763531]  iomap_readahead+0x1a8/0x870
[17309.767550]  ? iomap_read_folio+0x4c0/0x4c0
[17309.771817]  ? lockdep_hardirqs_on_prepare+0x400/0x400
[17309.778848]  ? lock_release+0x370/0x750
[17309.784462]  ? folio_add_lru+0x217/0x3f0
[17309.790220]  ? reacquire_held_locks+0x4e0/0x4e0
[17309.796543]  read_pages+0x17d/0xb60
[17309.801854]  ? folio_add_lru+0x238/0x3f0
[17309.807573]  ? readahead_expand+0x5f0/0x5f0
[17309.813554]  ? policy_node+0xb5/0x140
[17309.819018]  page_cache_ra_unbounded+0x27d/0x450
[17309.825439]  filemap_get_pages+0x500/0x1450
[17309.831444]  ? filemap_add_folio+0x140/0x140
[17309.837519]  ? lock_is_held_type+0xd8/0x130
[17309.843509]  filemap_read+0x28c/0x9f0
[17309.848953]  ? zonefs_file_read_iter+0x1ea/0x4d0 [zonefs]
[17309.856162]  ? trace_contention_end+0xd6/0x130
[17309.862416]  ? __mutex_lock+0x221/0x1480
[17309.868151]  ? zonefs_file_read_iter+0x166/0x4d0 [zonefs]
[17309.875364]  ? filemap_get_pages+0x1450/0x1450
[17309.881647]  ? __mutex_unlock_slowpath+0x15e/0x620
[17309.888248]  ? wait_for_completion_io_timeout+0x20/0x20
[17309.895231]  ? lock_is_held_type+0xd8/0x130
[17309.901115]  ? lock_is_held_type+0xd8/0x130
[17309.906934]  zonefs_file_read_iter+0x356/0x4d0 [zonefs]
[17309.913750]  new_sync_read+0x2d8/0x520
[17309.919035]  ? __x64_sys_lseek+0x1d0/0x1d0

Furthermore, this causes iomap_readahead() to loop forever as
iomap_readahead_iter() always return 0, making no progress.

Fix this by avoiding that the iomap length be calculated as 0 by not
modifying the original length argument passed to zonefs_iomap_begin().

Reported-by: Jorgen Hansen <Jorgen.Hansen@wdc.com>
Fixes: 8dcc1a9d90c1 ("fs: New zonefs file system")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 fs/zonefs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index 123464d2145a..64f4ceb6f579 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -144,7 +144,7 @@ static int zonefs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 		iomap->type = IOMAP_MAPPED;
 	if (flags & IOMAP_WRITE)
 		length = zi->i_max_size - offset;
-	else
+	else if (offset < isize)
 		length = min(length, isize - offset);
 	mutex_unlock(&zi->i_truncate_mutex);
 
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] zonefs: fix handling of explicit_open option on mount
  2022-06-03 11:49 ` [PATCH 1/3] zonefs: fix handling of explicit_open option on mount Damien Le Moal
@ 2022-06-07  6:01   ` Christoph Hellwig
  2022-06-07  8:46   ` Johannes Thumshirn
  1 sibling, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2022-06-07  6:01 UTC (permalink / raw)
  To: Damien Le Moal; +Cc: linux-fsdevel, Johannes Thumshirn

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] zonefs: Do not ignore explicit_open with active zone limit
  2022-06-03 11:49 ` [PATCH 2/3] zonefs: Do not ignore explicit_open with active zone limit Damien Le Moal
@ 2022-06-07  6:02   ` Christoph Hellwig
  0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2022-06-07  6:02 UTC (permalink / raw)
  To: Damien Le Moal; +Cc: linux-fsdevel, Johannes Thumshirn

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] zonefs: fix zonefs_iomap_begin() for reads
  2022-06-03 11:49 ` [PATCH 3/3] zonefs: fix zonefs_iomap_begin() for reads Damien Le Moal
@ 2022-06-07  6:09   ` Christoph Hellwig
  2022-06-07  6:53     ` Damien Le Moal
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2022-06-07  6:09 UTC (permalink / raw)
  To: Damien Le Moal; +Cc: linux-fsdevel, Johannes Thumshirn

On Fri, Jun 03, 2022 at 08:49:39PM +0900, Damien Le Moal wrote:
> If a read operation (e.g. a readahead) is issued to a sequential zone
> file with an offset exactly equal to the current file size, the iomap
> type will be set to IOMAP_UNWRITTEN, which will prevent an IO, but the
> iomap length is always calculated as 0. This causes a WARN_ON() in
> iomap_iter():

Is there a testsuite somewhere with a reproducer?

> diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
> index 123464d2145a..64f4ceb6f579 100644
> --- a/fs/zonefs/super.c
> +++ b/fs/zonefs/super.c
> @@ -144,7 +144,7 @@ static int zonefs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>  		iomap->type = IOMAP_MAPPED;
>  	if (flags & IOMAP_WRITE)
>  		length = zi->i_max_size - offset;
> -	else
> +	else if (offset < isize)
>  		length = min(length, isize - offset);

So you still report an IOMAP_UNWRITTEN extent for the whole size of
the requst past EOF?  Looking at XFS we do return the whole requested
length, but do return it as HOLE.  Maybe we need to clarify the behavior
here and document it.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] zonefs: fix zonefs_iomap_begin() for reads
  2022-06-07  6:09   ` Christoph Hellwig
@ 2022-06-07  6:53     ` Damien Le Moal
  2022-06-07 10:09       ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Damien Le Moal @ 2022-06-07  6:53 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-fsdevel, Johannes Thumshirn

On 2022/06/07 15:09, Christoph Hellwig wrote:
> On Fri, Jun 03, 2022 at 08:49:39PM +0900, Damien Le Moal wrote:
>> If a read operation (e.g. a readahead) is issued to a sequential zone
>> file with an offset exactly equal to the current file size, the iomap
>> type will be set to IOMAP_UNWRITTEN, which will prevent an IO, but the
>> iomap length is always calculated as 0. This causes a WARN_ON() in
>> iomap_iter():
> 
> Is there a testsuite somewhere with a reproducer?

Yes, test case 0325 of the test suite that comes with zonefs-tools:

git@github.com:westerndigitalcorporation/zonefs-tools.git

The tests are under the "tests" directory and running:

zonefs-tests-nullblk.sh -t 0325

triggers the problem 100% of the time.

> 
>> diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
>> index 123464d2145a..64f4ceb6f579 100644
>> --- a/fs/zonefs/super.c
>> +++ b/fs/zonefs/super.c
>> @@ -144,7 +144,7 @@ static int zonefs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>>  		iomap->type = IOMAP_MAPPED;
>>  	if (flags & IOMAP_WRITE)
>>  		length = zi->i_max_size - offset;
>> -	else
>> +	else if (offset < isize)
>>  		length = min(length, isize - offset);
> 
> So you still report an IOMAP_UNWRITTEN extent for the whole size of
> the requst past EOF?  Looking at XFS we do return the whole requested
> length, but do return it as HOLE.  Maybe we need to clarify the behavior
> here and document it.

Yes, I checked xfs and saw that. But in zonefs case, since the file blocks are
always preallocated, blocks after the write pointer are indeed UNWRITTEN. I did
check that iomap read processing treats UNWRITTEN and HOLE similarly, that is,
ignore the value of length and stop issuing IOs when either type is seen. But I
may have missed something.

Note that initially I wrote the patch below to fix the problem. But it is very
large and should probably be a cleanup for the next cycle. It separates the
begin op for read and write, which makes things easier to understand.



From 1e1024daff9158f36fe01328c4be83db940d4309 Mon Sep 17 00:00:00 2001
From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Date: Mon, 23 May 2022 16:29:10 +0900
Subject: [PATCH] zonefs: fix iomap_begin operation


Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 fs/zonefs/super.c | 103 +++++++++++++++++++++++++++++-----------------
 1 file changed, 66 insertions(+), 37 deletions(-)

diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index 123464d2145a..29c609aede65 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -110,15 +110,48 @@ static inline void zonefs_i_size_write(struct inode
*inode, loff_t isize)
 	}
 }

-static int zonefs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
-			      unsigned int flags, struct iomap *iomap,
-			      struct iomap *srcmap)
+static int zonefs_read_iomap_begin(struct inode *inode, loff_t offset,
+				   loff_t length, unsigned int flags,
+				   struct iomap *iomap, struct iomap *srcmap)
 {
 	struct zonefs_inode_info *zi = ZONEFS_I(inode);
 	struct super_block *sb = inode->i_sb;
-	loff_t isize;
+	loff_t isize = i_size_read(inode);
+
+	/*
+	 * All blocks are always mapped below EOF. If reading past EOF,
+	 * act as if there is a hole up to the file maximum size.
+	 */
+	iomap->bdev = inode->i_sb->s_bdev;
+	iomap->offset = ALIGN_DOWN(offset, sb->s_blocksize);
+	if (iomap->offset >= isize) {
+		iomap->type = IOMAP_HOLE;
+		iomap->addr = IOMAP_NULL_ADDR;
+		iomap->length = length;
+	} else {
+		iomap->type = IOMAP_MAPPED;
+		iomap->addr = (zi->i_zsector << SECTOR_SHIFT) + iomap->offset;
+		iomap->length = min(length, isize - iomap->offset);
+	}
+
+	trace_zonefs_iomap_begin(inode, iomap);
+
+	return 0;
+}
+
+static const struct iomap_ops zonefs_read_iomap_ops = {
+	.iomap_begin	= zonefs_read_iomap_begin,
+};
+
+static int zonefs_write_iomap_begin(struct inode *inode, loff_t offset,
+				    loff_t length, unsigned int flags,
+				    struct iomap *iomap, struct iomap *srcmap)
+{
+	struct zonefs_inode_info *zi = ZONEFS_I(inode);
+	struct super_block *sb = inode->i_sb;
+	loff_t remaining, isize = i_size_read(inode);

-	/* All I/Os should always be within the file maximum size */
+	/* All write I/Os should always be within the file maximum size */
 	if (WARN_ON_ONCE(offset + length > zi->i_max_size))
 		return -EIO;

@@ -128,56 +161,51 @@ static int zonefs_iomap_begin(struct inode *inode, loff_t
offset, loff_t length,
 	 * operation.
 	 */
 	if (WARN_ON_ONCE(zi->i_ztype == ZONEFS_ZTYPE_SEQ &&
-			 (flags & IOMAP_WRITE) && !(flags & IOMAP_DIRECT)))
+			 !(flags & IOMAP_DIRECT)))
 		return -EIO;

 	/*
-	 * For conventional zones, all blocks are always mapped. For sequential
-	 * zones, all blocks after always mapped below the inode size (zone
-	 * write pointer) and unwriten beyond.
+	 * For conventional zones, since the inode size is fixed, all blocks
+	 * are always mapped. For sequential zones, all blocks after always
+	 * mapped below the inode size (zone write pointer) and unwriten beyond.
 	 */
-	mutex_lock(&zi->i_truncate_mutex);
-	isize = i_size_read(inode);
-	if (offset >= isize)
-		iomap->type = IOMAP_UNWRITTEN;
-	else
-		iomap->type = IOMAP_MAPPED;
-	if (flags & IOMAP_WRITE)
-		length = zi->i_max_size - offset;
-	else
-		length = min(length, isize - offset);
-	mutex_unlock(&zi->i_truncate_mutex);
-
-	iomap->offset = ALIGN_DOWN(offset, sb->s_blocksize);
-	iomap->length = ALIGN(offset + length, sb->s_blocksize) - iomap->offset;
 	iomap->bdev = inode->i_sb->s_bdev;
+	iomap->offset = ALIGN_DOWN(offset, sb->s_blocksize);
 	iomap->addr = (zi->i_zsector << SECTOR_SHIFT) + iomap->offset;
+	if (iomap->offset >= isize) {
+		iomap->type = IOMAP_UNWRITTEN;
+		remaining = zi->i_max_size - iomap->offset;
+	} else {
+		iomap->type = IOMAP_MAPPED;
+		remaining = isize - iomap->offset;
+	}
+	iomap->length = min(length, remaining);

 	trace_zonefs_iomap_begin(inode, iomap);

 	return 0;
 }

-static const struct iomap_ops zonefs_iomap_ops = {
-	.iomap_begin	= zonefs_iomap_begin,
+static const struct iomap_ops zonefs_write_iomap_ops = {
+	.iomap_begin	= zonefs_write_iomap_begin,
 };

 static int zonefs_read_folio(struct file *unused, struct folio *folio)
 {
-	return iomap_read_folio(folio, &zonefs_iomap_ops);
+	return iomap_read_folio(folio, &zonefs_read_iomap_ops);
 }

 static void zonefs_readahead(struct readahead_control *rac)
 {
-	iomap_readahead(rac, &zonefs_iomap_ops);
+	iomap_readahead(rac, &zonefs_read_iomap_ops);
 }

 /*
  * Map blocks for page writeback. This is used only on conventional zone files,
  * which implies that the page range can only be within the fixed inode size.
  */
-static int zonefs_map_blocks(struct iomap_writepage_ctx *wpc,
-			     struct inode *inode, loff_t offset)
+static int zonefs_write_map_blocks(struct iomap_writepage_ctx *wpc,
+				   struct inode *inode, loff_t offset)
 {
 	struct zonefs_inode_info *zi = ZONEFS_I(inode);

@@ -191,12 +219,12 @@ static int zonefs_map_blocks(struct iomap_writepage_ctx *wpc,
 	    offset < wpc->iomap.offset + wpc->iomap.length)
 		return 0;

-	return zonefs_iomap_begin(inode, offset, zi->i_max_size - offset,
-				  IOMAP_WRITE, &wpc->iomap, NULL);
+	return zonefs_write_iomap_begin(inode, offset, zi->i_max_size - offset,
+					IOMAP_WRITE, &wpc->iomap, NULL);
 }

 static const struct iomap_writeback_ops zonefs_writeback_ops = {
-	.map_blocks		= zonefs_map_blocks,
+	.map_blocks		= zonefs_write_map_blocks,
 };

 static int zonefs_writepage(struct page *page, struct writeback_control *wbc)
@@ -226,7 +254,8 @@ static int zonefs_swap_activate(struct swap_info_struct *sis,
 		return -EINVAL;
 	}

-	return iomap_swapfile_activate(sis, swap_file, span, &zonefs_iomap_ops);
+	return iomap_swapfile_activate(sis, swap_file, span,
+				       &zonefs_read_iomap_ops);
 }

 static const struct address_space_operations zonefs_file_aops = {
@@ -647,7 +676,7 @@ static vm_fault_t zonefs_filemap_page_mkwrite(struct
vm_fault *vmf)

 	/* Serialize against truncates */
 	filemap_invalidate_lock_shared(inode->i_mapping);
-	ret = iomap_page_mkwrite(vmf, &zonefs_iomap_ops);
+	ret = iomap_page_mkwrite(vmf, &zonefs_write_iomap_ops);
 	filemap_invalidate_unlock_shared(inode->i_mapping);

 	sb_end_pagefault(inode->i_sb);
@@ -899,7 +928,7 @@ static ssize_t zonefs_file_dio_write(struct kiocb *iocb,
struct iov_iter *from)
 	if (append)
 		ret = zonefs_file_dio_append(iocb, from);
 	else
-		ret = iomap_dio_rw(iocb, from, &zonefs_iomap_ops,
+		ret = iomap_dio_rw(iocb, from, &zonefs_write_iomap_ops,
 				   &zonefs_write_dio_ops, 0, NULL, 0);
 	if (zi->i_ztype == ZONEFS_ZTYPE_SEQ &&
 	    (ret > 0 || ret == -EIOCBQUEUED)) {
@@ -948,7 +977,7 @@ static ssize_t zonefs_file_buffered_write(struct kiocb *iocb,
 	if (ret <= 0)
 		goto inode_unlock;

-	ret = iomap_file_buffered_write(iocb, from, &zonefs_iomap_ops);
+	ret = iomap_file_buffered_write(iocb, from, &zonefs_write_iomap_ops);
 	if (ret > 0)
 		iocb->ki_pos += ret;
 	else if (ret == -EIO)
@@ -1041,7 +1070,7 @@ static ssize_t zonefs_file_read_iter(struct kiocb *iocb,
struct iov_iter *to)
 			goto inode_unlock;
 		}
 		file_accessed(iocb->ki_filp);
-		ret = iomap_dio_rw(iocb, to, &zonefs_iomap_ops,
+		ret = iomap_dio_rw(iocb, to, &zonefs_read_iomap_ops,
 				   &zonefs_read_dio_ops, 0, NULL, 0);
 	} else {
 		ret = generic_file_read_iter(iocb, to);
-- 
2.36.1



-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] zonefs: fix handling of explicit_open option on mount
  2022-06-03 11:49 ` [PATCH 1/3] zonefs: fix handling of explicit_open option on mount Damien Le Moal
  2022-06-07  6:01   ` Christoph Hellwig
@ 2022-06-07  8:46   ` Johannes Thumshirn
  1 sibling, 0 replies; 11+ messages in thread
From: Johannes Thumshirn @ 2022-06-07  8:46 UTC (permalink / raw)
  To: Damien Le Moal, linux-fsdevel

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] zonefs: fix zonefs_iomap_begin() for reads
  2022-06-07  6:53     ` Damien Le Moal
@ 2022-06-07 10:09       ` Christoph Hellwig
  2022-06-07 10:25         ` Damien Le Moal
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2022-06-07 10:09 UTC (permalink / raw)
  To: Damien Le Moal; +Cc: Christoph Hellwig, linux-fsdevel, Johannes Thumshirn

On Tue, Jun 07, 2022 at 03:53:35PM +0900, Damien Le Moal wrote:
> >> +	else if (offset < isize)
> >>  		length = min(length, isize - offset);
> > 
> > So you still report an IOMAP_UNWRITTEN extent for the whole size of
> > the requst past EOF?  Looking at XFS we do return the whole requested
> > length, but do return it as HOLE.  Maybe we need to clarify the behavior
> > here and document it.
> 
> Yes, I checked xfs and saw that. But in zonefs case, since the file blocks are
> always preallocated, blocks after the write pointer are indeed UNWRITTEN. I did
> check that iomap read processing treats UNWRITTEN and HOLE similarly, that is,
> ignore the value of length and stop issuing IOs when either type is seen. But I
> may have missed something.
> 
> Note that initially I wrote the patch below to fix the problem. But it is very
> large and should probably be a cleanup for the next cycle. It separates the
> begin op for read and write, which makes things easier to understand.

I much prefer this more extensive fix.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] zonefs: fix zonefs_iomap_begin() for reads
  2022-06-07 10:09       ` Christoph Hellwig
@ 2022-06-07 10:25         ` Damien Le Moal
  0 siblings, 0 replies; 11+ messages in thread
From: Damien Le Moal @ 2022-06-07 10:25 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-fsdevel, Johannes Thumshirn

On 6/7/22 19:09, Christoph Hellwig wrote:
> On Tue, Jun 07, 2022 at 03:53:35PM +0900, Damien Le Moal wrote:
>>>> +	else if (offset < isize)
>>>>   		length = min(length, isize - offset);
>>>
>>> So you still report an IOMAP_UNWRITTEN extent for the whole size of
>>> the requst past EOF?  Looking at XFS we do return the whole requested
>>> length, but do return it as HOLE.  Maybe we need to clarify the behavior
>>> here and document it.
>>
>> Yes, I checked xfs and saw that. But in zonefs case, since the file blocks are
>> always preallocated, blocks after the write pointer are indeed UNWRITTEN. I did
>> check that iomap read processing treats UNWRITTEN and HOLE similarly, that is,
>> ignore the value of length and stop issuing IOs when either type is seen. But I
>> may have missed something.
>>
>> Note that initially I wrote the patch below to fix the problem. But it is very
>> large and should probably be a cleanup for the next cycle. It separates the
>> begin op for read and write, which makes things easier to understand.
> 
> I much prefer this more extensive fix.

OK. Will resend with this change. It does make the code a lot cleaner.


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-06-07 10:25 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-03 11:49 [PATCH 0/3] zonefs fixes Damien Le Moal
2022-06-03 11:49 ` [PATCH 1/3] zonefs: fix handling of explicit_open option on mount Damien Le Moal
2022-06-07  6:01   ` Christoph Hellwig
2022-06-07  8:46   ` Johannes Thumshirn
2022-06-03 11:49 ` [PATCH 2/3] zonefs: Do not ignore explicit_open with active zone limit Damien Le Moal
2022-06-07  6:02   ` Christoph Hellwig
2022-06-03 11:49 ` [PATCH 3/3] zonefs: fix zonefs_iomap_begin() for reads Damien Le Moal
2022-06-07  6:09   ` Christoph Hellwig
2022-06-07  6:53     ` Damien Le Moal
2022-06-07 10:09       ` Christoph Hellwig
2022-06-07 10:25         ` Damien Le Moal

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.