[RFC PATCH 0/9] Convert JFS to use iomap

[RFC PATCH 0/9] Convert JFS to use iomap

[Matthew Wilcox (Oracle)]

This patchset does not work.  It will eat your filesystem.  Do not apply.

The bug starts to show up with the fourth patch ("Convert direct_IO write
support to use iomap").  generic/013 creates a corrupt filesystem and
fsck fails to fix it, which shows all kinds of fun places in xfstests
where we neglect to check that 'mount' actually mounted the filesystem.
set -x or die.

I'm hoping one of the people who knows iomap better than I do can just
point at the bug and say "Duh, it doesn't work like that".

It's safe to say that every patch after patch 6 is untested.  I'm not
convinced that I really tested patch 6 either.

Matthew Wilcox (Oracle) (9):
  IOMAP_DIO_NOSYNC
  jfs: Add jfs_iomap_begin()
  jfs: Convert direct_IO read support to use iomap
  jfs: Convert direct_IO write support to use iomap
  jfs: Remove old direct_IO support
  jfs: Handle bmap with iomap
  jfs: Read quota through the page cache
  jfs: Write quota through the page cache
  jfs: Convert buffered IO paths to iomap

 fs/iomap/direct-io.c  |   3 +-
 fs/jfs/file.c         |  56 +++++++++++++++++-
 fs/jfs/inode.c        | 128 ++++++++++++++++--------------------------
 fs/jfs/jfs_inode.h    |   2 +-
 fs/jfs/jfs_logmgr.c   |   1 -
 fs/jfs/jfs_metapage.c |   1 -
 fs/jfs/super.c        | 127 +++++++++++++++++++----------------------
 include/linux/iomap.h |   6 ++
 8 files changed, 168 insertions(+), 156 deletions(-)

-- 
2.34.1

[RFC PATCH 1/9] IOMAP_DIO_NOSYNC

[Matthew Wilcox (Oracle)]

Al's patch.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/iomap/direct-io.c  | 3 ++-
 include/linux/iomap.h | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 370c3241618a..6a361131080f 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -548,7 +548,8 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		}
 
 		/* for data sync or sync, we need sync completion processing */
-		if (iocb->ki_flags & IOCB_DSYNC)
+		if (iocb->ki_flags & IOCB_DSYNC &&
+		    !(dio_flags & IOMAP_DIO_NOSYNC))
 			dio->flags |= IOMAP_DIO_NEED_SYNC;
 
 		/*
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index e552097c67e0..c8622d8f064e 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -353,6 +353,12 @@ struct iomap_dio_ops {
  */
 #define IOMAP_DIO_PARTIAL		(1 << 2)
 
+/*
+ * The caller will sync the write if needed; do not sync it within
+ * iomap_dio_rw.  Overrides IOMAP_DIO_FORCE_WAIT.
+ */
+#define IOMAP_DIO_NOSYNC		(1 << 3)
+
 ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		const struct iomap_ops *ops, const struct iomap_dio_ops *dops,
 		unsigned int dio_flags, void *private, size_t done_before);
-- 
2.34.1

Re: [RFC PATCH 1/9] IOMAP_DIO_NOSYNC

[Christoph Hellwig]

On Thu, May 26, 2022 at 08:29:02PM +0100, Matthew Wilcox (Oracle) wrote:
> Al's patch.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Hmm, that is a bit of a weird changelog..

>  		/* for data sync or sync, we need sync completion processing */
> -		if (iocb->ki_flags & IOCB_DSYNC)
> +		if (iocb->ki_flags & IOCB_DSYNC &&
> +		    !(dio_flags & IOMAP_DIO_NOSYNC))
>  			dio->flags |= IOMAP_DIO_NEED_SYNC;
>  
>  		/*

I think we also need to skip the setting of IOMAP_DIO_WRITE_FUA below
(or find a way to communicate back to the that FUA was used, which
seems way more complicated)

Re: [RFC PATCH 1/9] IOMAP_DIO_NOSYNC

[Matthew Wilcox]

On Thu, May 26, 2022 at 10:35:11PM -0700, Christoph Hellwig wrote:
> On Thu, May 26, 2022 at 08:29:02PM +0100, Matthew Wilcox (Oracle) wrote:
> > Al's patch.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Hmm, that is a bit of a weird changelog..

I took parts of
https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=new.iov_iter&id=447262443a4dbf88ae5b21de5c77197f084cdca4

and fixed it up a bit but dropped the btrfs parts.

> >  		/* for data sync or sync, we need sync completion processing */
> > -		if (iocb->ki_flags & IOCB_DSYNC)
> > +		if (iocb->ki_flags & IOCB_DSYNC &&
> > +		    !(dio_flags & IOMAP_DIO_NOSYNC))
> >  			dio->flags |= IOMAP_DIO_NEED_SYNC;
> >  
> >  		/*
> 
> I think we also need to skip the setting of IOMAP_DIO_WRITE_FUA below
> (or find a way to communicate back to the that FUA was used, which
> seems way more complicated)

Probably ... I was just looking to avoid the deadlock.  Patch not for
upstream in this form; I'm expecting Al to submit a fixed version before
the JFS code is ready for upstream.

[RFC PATCH 2/9] jfs: Add jfs_iomap_begin()

[Matthew Wilcox (Oracle)]

Turn jfs_get_block() into a wrapper around jfs_iomap_begin().  This fixes
a latent bug where JFS was not setting b_size when it encountered a hole.
At least mpage_readahead() does not look at b_size when the buffer is
unmapped, but iomap will care whether iomap->length is set correctly,
and so we may as well set b_size correctly as well.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/jfs/inode.c | 67 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 17 deletions(-)

diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
index a5dd7e53754a..1a5bdaf35e9b 100644
--- a/fs/jfs/inode.c
+++ b/fs/jfs/inode.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/fs.h>
+#include <linux/iomap.h>
 #include <linux/mpage.h>
 #include <linux/buffer_head.h>
 #include <linux/pagemap.h>
@@ -196,15 +197,21 @@ void jfs_dirty_inode(struct inode *inode, int flags)
 	set_cflag(COMMIT_Dirty, inode);
 }
 
-int jfs_get_block(struct inode *ip, sector_t lblock,
-		  struct buffer_head *bh_result, int create)
+int jfs_iomap_begin(struct inode *ip, loff_t pos, loff_t length,
+		unsigned flags, struct iomap *iomap, struct iomap *srcmap)
 {
-	s64 lblock64 = lblock;
+	s64 lblock64 = pos >> ip->i_blkbits;
+	int create = flags & IOMAP_WRITE;
 	int rc = 0;
 	xad_t xad;
 	s64 xaddr;
 	int xflag;
-	s32 xlen = bh_result->b_size >> ip->i_blkbits;
+	s32 xlen;
+
+	xlen = DIV_ROUND_UP(pos + length - (lblock64 << ip->i_blkbits),
+			    i_blocksize(ip));
+	if (xlen < 0)
+		xlen = INT_MAX;
 
 	/*
 	 * Take appropriate lock on inode
@@ -214,8 +221,8 @@ int jfs_get_block(struct inode *ip, sector_t lblock,
 	else
 		IREAD_LOCK(ip, RDWRLOCK_NORMAL);
 
-	if (((lblock64 << ip->i_sb->s_blocksize_bits) < ip->i_size) &&
-	    (!xtLookup(ip, lblock64, xlen, &xflag, &xaddr, &xlen, 0)) &&
+	if (pos < ip->i_size &&
+	    !xtLookup(ip, lblock64, xlen, &xflag, &xaddr, &xlen, 0) &&
 	    xaddr) {
 		if (xflag & XAD_NOTRECORDED) {
 			if (!create)
@@ -238,13 +245,11 @@ int jfs_get_block(struct inode *ip, sector_t lblock,
 #endif				/* _JFS_4K */
 			rc = extRecord(ip, &xad);
 			if (rc)
-				goto unlock;
-			set_buffer_new(bh_result);
+				goto err;
+			iomap->flags |= IOMAP_F_NEW;
 		}
 
-		map_bh(bh_result, ip->i_sb, xaddr);
-		bh_result->b_size = xlen << ip->i_blkbits;
-		goto unlock;
+		goto map;
 	}
 	if (!create)
 		goto unlock;
@@ -254,14 +259,14 @@ int jfs_get_block(struct inode *ip, sector_t lblock,
 	 */
 #ifdef _JFS_4K
 	if ((rc = extHint(ip, lblock64 << ip->i_sb->s_blocksize_bits, &xad)))
-		goto unlock;
+		goto err;
 	rc = extAlloc(ip, xlen, lblock64, &xad, false);
 	if (rc)
-		goto unlock;
+		goto err;
 
-	set_buffer_new(bh_result);
-	map_bh(bh_result, ip->i_sb, addressXAD(&xad));
-	bh_result->b_size = lengthXAD(&xad) << ip->i_blkbits;
+	xaddr = addressXAD(&xad);
+	xlen = lengthXAD(&xad);
+	iomap->flags |= IOMAP_F_NEW;
 
 #else				/* _JFS_4K */
 	/*
@@ -271,7 +276,14 @@ int jfs_get_block(struct inode *ip, sector_t lblock,
 	BUG();
 #endif				/* _JFS_4K */
 
-      unlock:
+map:
+	iomap->addr = xaddr << ip->i_blkbits;
+	iomap->bdev = ip->i_sb->s_bdev;
+	iomap->type = IOMAP_MAPPED;
+unlock:
+	iomap->offset = lblock64 << ip->i_blkbits;
+	iomap->length = xlen << ip->i_blkbits;
+err:
 	/*
 	 * Release lock on inode
 	 */
@@ -282,6 +294,27 @@ int jfs_get_block(struct inode *ip, sector_t lblock,
 	return rc;
 }
 
+int jfs_get_block(struct inode *ip, sector_t lblock,
+		  struct buffer_head *bh_result, int create)
+{
+	struct iomap iomap = { };
+	int ret;
+
+	ret = jfs_iomap_begin(ip, lblock << ip->i_blkbits, bh_result->b_size,
+			create ? IOMAP_WRITE : 0, &iomap, NULL);
+	if (ret)
+		return ret;
+
+	bh_result->b_size = iomap.length;
+	if (iomap.type == IOMAP_HOLE)
+		return 0;
+
+	map_bh(bh_result, ip->i_sb, iomap.addr >> ip->i_blkbits);
+	if (iomap.flags & IOMAP_F_NEW)
+		set_buffer_new(bh_result);
+	return 0;
+}
+
 static int jfs_writepage(struct page *page, struct writeback_control *wbc)
 {
 	return block_write_full_page(page, jfs_get_block, wbc);
-- 
2.34.1

Re: [RFC PATCH 2/9] jfs: Add jfs_iomap_begin()

[Christoph Hellwig]

I suspect this might be where your problems lies:

blockdev_direct_IO calls __blockdev_direct_IO with DIO_SKIP_HOLES set.
DIO_SKIP_HOLES causes get_more_blocks to never set the create bit
to get_block except for writes beyond i_size.  If we want to replicate
that behavior with iomap, ->iomap_begin needs to return -ENOTBLK
when it encounters a hole for writing.  To properly supporting writing
to holes we'd need unwritten extents, which jfs does not support.
gfs2 might be a place to look for how to implement this.

Re: [RFC PATCH 2/9] jfs: Add jfs_iomap_begin()

[Matthew Wilcox]

On Thu, May 26, 2022 at 10:41:31PM -0700, Christoph Hellwig wrote:
> I suspect this might be where your problems lies:
> 
> blockdev_direct_IO calls __blockdev_direct_IO with DIO_SKIP_HOLES set.
> DIO_SKIP_HOLES causes get_more_blocks to never set the create bit
> to get_block except for writes beyond i_size.  If we want to replicate
> that behavior with iomap, ->iomap_begin needs to return -ENOTBLK
> when it encounters a hole for writing.  To properly supporting writing
> to holes we'd need unwritten extents, which jfs does not support.
> gfs2 might be a place to look for how to implement this.

I think JFS does support unwritten extents,
fs/jfs/jfs_xtree.h:#define XAD_NOTRECORDED 0x08 /* allocated but not recorded */

However, we always pass 'false' to extAlloc() today, so I think it
hasn't been tested in a while?  I'm not sure I want to be the one to
start using new features on JFS for something that's supposed to be
a relatively quick cleanup.

Re: [Jfs-discussion] [RFC PATCH 2/9] jfs: Add jfs_iomap_begin()

[Dave Kleikamp]

On 5/27/22 8:45AM, Matthew Wilcox wrote:
> On Thu, May 26, 2022 at 10:41:31PM -0700, Christoph Hellwig wrote:
>> I suspect this might be where your problems lies:
>>
>> blockdev_direct_IO calls __blockdev_direct_IO with DIO_SKIP_HOLES set.
>> DIO_SKIP_HOLES causes get_more_blocks to never set the create bit
>> to get_block except for writes beyond i_size.  If we want to replicate
>> that behavior with iomap, ->iomap_begin needs to return -ENOTBLK
>> when it encounters a hole for writing.  To properly supporting writing
>> to holes we'd need unwritten extents, which jfs does not support.
>> gfs2 might be a place to look for how to implement this.
> 
> I think JFS does support unwritten extents,
> fs/jfs/jfs_xtree.h:#define XAD_NOTRECORDED 0x08 /* allocated but not recorded */
> 
> However, we always pass 'false' to extAlloc() today, so I think it
> hasn't been tested in a while?  I'm not sure I want to be the one to
> start using new features on JFS for something that's supposed to be
> a relatively quick cleanup.

If I remember correctly, there was an intention to implement unwritten 
extents in the future, but it never got implemented. We tried to 
anticipate the unwritten extents in the existing code as much as possible.

Shaggy

[RFC PATCH 3/9] jfs: Convert direct_IO read support to use iomap

[Matthew Wilcox (Oracle)]

Use the new iomap support to handle direct IO reads.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/jfs/file.c      | 20 ++++++++++++++++++--
 fs/jfs/inode.c     |  4 ++++
 fs/jfs/jfs_inode.h |  1 +
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 1d732fd223d4..0d074a9e0f77 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -4,10 +4,12 @@
  *   Portions Copyright (C) Christoph Hellwig, 2001-2002
  */
 
-#include <linux/mm.h>
 #include <linux/fs.h>
+#include <linux/iomap.h>
+#include <linux/mm.h>
 #include <linux/posix_acl.h>
 #include <linux/quotaops.h>
+#include <linux/uio.h>
 #include "jfs_incore.h"
 #include "jfs_inode.h"
 #include "jfs_dmap.h"
@@ -70,6 +72,20 @@ static int jfs_open(struct inode *inode, struct file *file)
 
 	return 0;
 }
+
+static ssize_t jfs_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+	if (!iov_iter_count(to))
+		return 0; /* skip atime */
+
+	if (iocb->ki_flags & IOCB_DIRECT) {
+		file_accessed(iocb->ki_filp);
+		return iomap_dio_rw(iocb, to, &jfs_iomap_ops, NULL, 0, NULL, 0);
+	}
+
+	return generic_file_read_iter(iocb, to);
+}
+
 static int jfs_release(struct inode *inode, struct file *file)
 {
 	struct jfs_inode_info *ji = JFS_IP(inode);
@@ -141,7 +157,7 @@ const struct inode_operations jfs_file_inode_operations = {
 const struct file_operations jfs_file_operations = {
 	.open		= jfs_open,
 	.llseek		= generic_file_llseek,
-	.read_iter	= generic_file_read_iter,
+	.read_iter	= jfs_read_iter,
 	.write_iter	= generic_file_write_iter,
 	.mmap		= generic_file_mmap,
 	.splice_read	= generic_file_splice_read,
diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
index 1a5bdaf35e9b..22e8a5612fdc 100644
--- a/fs/jfs/inode.c
+++ b/fs/jfs/inode.c
@@ -294,6 +294,10 @@ int jfs_iomap_begin(struct inode *ip, loff_t pos, loff_t length,
 	return rc;
 }
 
+const struct iomap_ops jfs_iomap_ops = {
+	.iomap_begin =  jfs_iomap_begin,
+};
+
 int jfs_get_block(struct inode *ip, sector_t lblock,
 		  struct buffer_head *bh_result, int create)
 {
diff --git a/fs/jfs/jfs_inode.h b/fs/jfs/jfs_inode.h
index 7de961a81862..afd12de3c341 100644
--- a/fs/jfs/jfs_inode.h
+++ b/fs/jfs/jfs_inode.h
@@ -30,6 +30,7 @@ extern void jfs_set_inode_flags(struct inode *);
 extern int jfs_get_block(struct inode *, sector_t, struct buffer_head *, int);
 extern int jfs_setattr(struct user_namespace *, struct dentry *, struct iattr *);
 
+extern const struct iomap_ops jfs_iomap_ops;
 extern const struct address_space_operations jfs_aops;
 extern const struct inode_operations jfs_dir_inode_operations;
 extern const struct file_operations jfs_dir_operations;
-- 
2.34.1

[RFC PATCH 4/9] jfs: Convert direct_IO write support to use iomap

[Matthew Wilcox (Oracle)]

Use the new iomap support to handle direct IO writes.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/jfs/file.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 0d074a9e0f77..bf9d4475ddb5 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -4,6 +4,7 @@
  *   Portions Copyright (C) Christoph Hellwig, 2001-2002
  */
 
+#include <linux/backing-dev.h>
 #include <linux/fs.h>
 #include <linux/iomap.h>
 #include <linux/mm.h>
@@ -86,6 +87,39 @@ static ssize_t jfs_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	return generic_file_read_iter(iocb, to);
 }
 
+static ssize_t jfs_write_iter(struct kiocb *iocb, struct iov_iter *from)
+{
+	struct file *file = iocb->ki_filp;
+	struct inode *inode = file->f_mapping->host;
+	ssize_t ret;
+
+	inode_lock(inode);
+	current->backing_dev_info = inode_to_bdi(inode);
+
+	ret = generic_write_checks(iocb, from);
+	if (ret <= 0)
+		goto err;
+	ret = file_remove_privs(file);
+	if (ret < 0)
+		goto err;
+	ret = file_update_time(file);
+	if (ret < 0)
+		goto err;
+
+	if (iocb->ki_flags & IOCB_DIRECT)
+		ret = iomap_dio_rw(iocb, from, &jfs_iomap_ops, NULL,
+				IOMAP_DIO_NOSYNC, NULL, 0);
+	else
+		ret = __generic_file_write_iter(iocb, from);
+
+err:
+	current->backing_dev_info = NULL;
+	inode_unlock(inode);
+	if (ret > 0)
+		ret = generic_write_sync(iocb, ret);
+	return ret;
+}
+
 static int jfs_release(struct inode *inode, struct file *file)
 {
 	struct jfs_inode_info *ji = JFS_IP(inode);
@@ -158,7 +192,7 @@ const struct file_operations jfs_file_operations = {
 	.open		= jfs_open,
 	.llseek		= generic_file_llseek,
 	.read_iter	= jfs_read_iter,
-	.write_iter	= generic_file_write_iter,
+	.write_iter	= jfs_write_iter,
 	.mmap		= generic_file_mmap,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
-- 
2.34.1

[RFC PATCH 5/9] jfs: Remove old direct_IO support

[Matthew Wilcox (Oracle)]

Now that reads and writes both use iomap for O_DIRECT, this code has
no more callers and can be removed.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/jfs/inode.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
index 22e8a5612fdc..63690020cc46 100644
--- a/fs/jfs/inode.c
+++ b/fs/jfs/inode.c
@@ -368,31 +368,6 @@ static sector_t jfs_bmap(struct address_space *mapping, sector_t block)
 	return generic_block_bmap(mapping, block, jfs_get_block);
 }
 
-static ssize_t jfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
-{
-	struct file *file = iocb->ki_filp;
-	struct address_space *mapping = file->f_mapping;
-	struct inode *inode = file->f_mapping->host;
-	size_t count = iov_iter_count(iter);
-	ssize_t ret;
-
-	ret = blockdev_direct_IO(iocb, inode, iter, jfs_get_block);
-
-	/*
-	 * In case of error extending write may have instantiated a few
-	 * blocks outside i_size. Trim these off again.
-	 */
-	if (unlikely(iov_iter_rw(iter) == WRITE && ret < 0)) {
-		loff_t isize = i_size_read(inode);
-		loff_t end = iocb->ki_pos + count;
-
-		if (end > isize)
-			jfs_write_failed(mapping, end);
-	}
-
-	return ret;
-}
-
 const struct address_space_operations jfs_aops = {
 	.dirty_folio	= block_dirty_folio,
 	.invalidate_folio = block_invalidate_folio,
@@ -403,7 +378,7 @@ const struct address_space_operations jfs_aops = {
 	.write_begin	= jfs_write_begin,
 	.write_end	= nobh_write_end,
 	.bmap		= jfs_bmap,
-	.direct_IO	= jfs_direct_IO,
+	.direct_IO	= noop_direct_IO,
 };
 
 /*
-- 
2.34.1

[RFC PATCH 6/9] jfs: Handle bmap with iomap

[Matthew Wilcox (Oracle)]

Use the new iomap support to implement bmap.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/jfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
index 63690020cc46..19c091d9c20e 100644
--- a/fs/jfs/inode.c
+++ b/fs/jfs/inode.c
@@ -365,7 +365,7 @@ static int jfs_write_begin(struct file *file, struct address_space *mapping,
 
 static sector_t jfs_bmap(struct address_space *mapping, sector_t block)
 {
-	return generic_block_bmap(mapping, block, jfs_get_block);
+	return iomap_bmap(mapping, block, &jfs_iomap_ops);
 }
 
 const struct address_space_operations jfs_aops = {
-- 
2.34.1

[RFC PATCH 7/9] jfs: Read quota through the page cache

[Matthew Wilcox (Oracle)]

The comment above jfs_quota_read() is stale; sb_bread() will use the
page cache, so we may as well use the page cache directly and avoid
using jfs_get_block().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/jfs/super.c | 54 ++++++++++++++++++++------------------------------
 1 file changed, 21 insertions(+), 33 deletions(-)

diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 85d4f44f2ac4..1e7d117b555d 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -721,51 +721,39 @@ static int jfs_show_options(struct seq_file *seq, struct dentry *root)
 }
 
 #ifdef CONFIG_QUOTA
-
-/* Read data from quotafile - avoid pagecache and such because we cannot afford
- * acquiring the locks... As quota files are never truncated and quota code
- * itself serializes the operations (and no one else should touch the files)
- * we don't have to be afraid of races */
 static ssize_t jfs_quota_read(struct super_block *sb, int type, char *data,
-			      size_t len, loff_t off)
+			      size_t len, loff_t pos)
 {
 	struct inode *inode = sb_dqopt(sb)->files[type];
-	sector_t blk = off >> sb->s_blocksize_bits;
-	int err = 0;
-	int offset = off & (sb->s_blocksize - 1);
-	int tocopy;
+	struct address_space *mapping = inode->i_mapping;
 	size_t toread;
-	struct buffer_head tmp_bh;
-	struct buffer_head *bh;
+	pgoff_t index;
 	loff_t i_size = i_size_read(inode);
 
-	if (off > i_size)
+	if (pos > i_size)
 		return 0;
-	if (off+len > i_size)
-		len = i_size-off;
+	if (pos + len > i_size)
+		len = i_size - pos;
 	toread = len;
+	index = pos / PAGE_SIZE;
+
 	while (toread > 0) {
-		tocopy = sb->s_blocksize - offset < toread ?
-				sb->s_blocksize - offset : toread;
+		struct folio *folio = read_mapping_folio(mapping, index, NULL);
+		size_t tocopy = PAGE_SIZE - offset_in_page(pos);
+		void *src;
+
+		if (IS_ERR(folio))
+			return PTR_ERR(folio);
+
+		src = kmap_local_folio(folio, offset_in_folio(folio, pos));
+		memcpy(data, src, tocopy);
+		kunmap_local(src);
+		folio_put(folio);
 
-		tmp_bh.b_state = 0;
-		tmp_bh.b_size = i_blocksize(inode);
-		err = jfs_get_block(inode, blk, &tmp_bh, 0);
-		if (err)
-			return err;
-		if (!buffer_mapped(&tmp_bh))	/* A hole? */
-			memset(data, 0, tocopy);
-		else {
-			bh = sb_bread(sb, tmp_bh.b_blocknr);
-			if (!bh)
-				return -EIO;
-			memcpy(data, bh->b_data+offset, tocopy);
-			brelse(bh);
-		}
-		offset = 0;
 		toread -= tocopy;
 		data += tocopy;
-		blk++;
+		pos += tocopy;
+		index++;
 	}
 	return len;
 }
-- 
2.34.1

Re: [RFC PATCH 7/9] jfs: Read quota through the page cache

[Christoph Hellwig]

>  static ssize_t jfs_quota_read(struct super_block *sb, int type, char *data,
> +			      size_t len, loff_t pos)
>  {
>  	struct inode *inode = sb_dqopt(sb)->files[type];
> +	struct address_space *mapping = inode->i_mapping;
>  	size_t toread;
> +	pgoff_t index;
>  	loff_t i_size = i_size_read(inode);
>  
> +	if (pos > i_size)
>  		return 0;
> +	if (pos + len > i_size)
> +		len = i_size - pos;
>  	toread = len;
> +	index = pos / PAGE_SIZE;
> +
>  	while (toread > 0) {
> +		struct folio *folio = read_mapping_folio(mapping, index, NULL);
> +		size_t tocopy = PAGE_SIZE - offset_in_page(pos);
> +		void *src;
> +
> +		if (IS_ERR(folio))
> +			return PTR_ERR(folio);
> +
> +		src = kmap_local_folio(folio, offset_in_folio(folio, pos));
> +		memcpy(data, src, tocopy);
> +		kunmap_local(src);

It would be great to have a memcpy_from_folio like the existing
memcpy_from_page for this.

> +		folio_put(folio);
>  
>  		toread -= tocopy;
>  		data += tocopy;
> +		pos += tocopy;
> +		index++;
>  	}
>  	return len;

And this whole helper is generic now.  It might be worth to move it
into fs/quota/dquot.c as generic_quota_read.

Re: [RFC PATCH 7/9] jfs: Read quota through the page cache

[Matthew Wilcox]

On Thu, May 26, 2022 at 10:43:51PM -0700, Christoph Hellwig wrote:
> >  static ssize_t jfs_quota_read(struct super_block *sb, int type, char *data,
> > +			      size_t len, loff_t pos)
> >  {
> >  	struct inode *inode = sb_dqopt(sb)->files[type];
> > +	struct address_space *mapping = inode->i_mapping;
> >  	size_t toread;
> > +	pgoff_t index;
> >  	loff_t i_size = i_size_read(inode);
> >  
> > +	if (pos > i_size)
> >  		return 0;
> > +	if (pos + len > i_size)
> > +		len = i_size - pos;
> >  	toread = len;
> > +	index = pos / PAGE_SIZE;
> > +
> >  	while (toread > 0) {
> > +		struct folio *folio = read_mapping_folio(mapping, index, NULL);
> > +		size_t tocopy = PAGE_SIZE - offset_in_page(pos);
> > +		void *src;
> > +
> > +		if (IS_ERR(folio))
> > +			return PTR_ERR(folio);
> > +
> > +		src = kmap_local_folio(folio, offset_in_folio(folio, pos));
> > +		memcpy(data, src, tocopy);
> > +		kunmap_local(src);
> 
> It would be great to have a memcpy_from_folio like the existing
> memcpy_from_page for this.

Yes, I agree.  It could copy more than a single page like
zero_user_segments() does.

> > +		folio_put(folio);
> >  
> >  		toread -= tocopy;
> >  		data += tocopy;
> > +		pos += tocopy;
> > +		index++;
> >  	}
> >  	return len;
> 
> And this whole helper is generic now.  It might be worth to move it
> into fs/quota/dquot.c as generic_quota_read.

I was thinking it was filemap_read_kernel(inode, pos, dst, len)
but perhaps both of these things ...

generic_quota_read

[Matthew Wilcox]

On Thu, May 26, 2022 at 10:43:51PM -0700, Christoph Hellwig wrote:
> >  static ssize_t jfs_quota_read(struct super_block *sb, int type, char *data,
> > +			      size_t len, loff_t pos)
>
> And this whole helper is generic now.  It might be worth to move it
> into fs/quota/dquot.c as generic_quota_read.

I've been working on that this week.  Unfortunately, you have to convert
both quota_read and quota_write at the same time, it turns out.  At
least ext4_quota_write() uses the bdev's inode's page cache to back
the buffer_heads, so quota_read() and quota_write() are incoherent
with each other:

00017 gqr: mapping:00000000ee19acfb index:0x1 pos:0x1470 len:0x30
00017 4qw: mapping:000000007f9a811e index:0x18405 pos:0x1440 len:0x30

I don't know if there's a way around this.  Can't really use
read_mapping_folio() on the bdev's inode in generic_quota_read() -- the
blocks for a given page might be fragmented on disk.  I don't know
if there's a way to tell ext4_bread() to use the inode's page cache
instead of the bdev's.  And if we did that, would it even work as
being part of a transaction?

Re: generic_quota_read

[Jan Kara]

On Fri 03-06-22 15:40:13, Matthew Wilcox wrote:
> On Thu, May 26, 2022 at 10:43:51PM -0700, Christoph Hellwig wrote:
> > >  static ssize_t jfs_quota_read(struct super_block *sb, int type, char *data,
> > > +			      size_t len, loff_t pos)
> >
> > And this whole helper is generic now.  It might be worth to move it
> > into fs/quota/dquot.c as generic_quota_read.
> 
> I've been working on that this week.  Unfortunately, you have to convert
> both quota_read and quota_write at the same time, it turns out.  At
> least ext4_quota_write() uses the bdev's inode's page cache to back
> the buffer_heads, so quota_read() and quota_write() are incoherent
> with each other:
> 
> 00017 gqr: mapping:00000000ee19acfb index:0x1 pos:0x1470 len:0x30
> 00017 4qw: mapping:000000007f9a811e index:0x18405 pos:0x1440 len:0x30

Yes, reads and writes have to use the same cache. Otherwise bad things
happen...

> I don't know if there's a way around this.  Can't really use
> read_mapping_folio() on the bdev's inode in generic_quota_read() -- the
> blocks for a given page might be fragmented on disk.  I don't know
> if there's a way to tell ext4_bread() to use the inode's page cache
> instead of the bdev's.

There's no way for ext4_bread() to read from inode's page cache. And that
is deliberate - ext4_bread() is used for filesystem metadata (and quota is
treated as filesystem metadata) and we use bdev page cache for all the
metadata.

> And if we did that, would it even work as being part of a transaction?

In principle it could work because we would then treat quota as journalled
data and jbd2 supports that. But honestly, special-casing quota as
journalled data IMHO brings more hassle on the write side than it can save
by sharing some code on the read side.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 8/9] jfs: Write quota through the page cache

[Matthew Wilcox (Oracle)]

Remove call to jfs_get_block() and use the page cache directly instead
of wrapping it in a buffer_head.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/jfs/super.c | 71 ++++++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 34 deletions(-)

diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 1e7d117b555d..151c62e2a08f 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -14,7 +14,6 @@
 #include <linux/moduleparam.h>
 #include <linux/kthread.h>
 #include <linux/posix_acl.h>
-#include <linux/buffer_head.h>
 #include <linux/exportfs.h>
 #include <linux/crc32.h>
 #include <linux/slab.h>
@@ -760,54 +759,58 @@ static ssize_t jfs_quota_read(struct super_block *sb, int type, char *data,
 
 /* Write to quotafile */
 static ssize_t jfs_quota_write(struct super_block *sb, int type,
-			       const char *data, size_t len, loff_t off)
+			       const char *data, size_t len, loff_t pos)
 {
 	struct inode *inode = sb_dqopt(sb)->files[type];
-	sector_t blk = off >> sb->s_blocksize_bits;
+	struct address_space *mapping = inode->i_mapping;
 	int err = 0;
-	int offset = off & (sb->s_blocksize - 1);
-	int tocopy;
 	size_t towrite = len;
-	struct buffer_head tmp_bh;
-	struct buffer_head *bh;
 
 	inode_lock(inode);
 	while (towrite > 0) {
-		tocopy = sb->s_blocksize - offset < towrite ?
-				sb->s_blocksize - offset : towrite;
-
-		tmp_bh.b_state = 0;
-		tmp_bh.b_size = i_blocksize(inode);
-		err = jfs_get_block(inode, blk, &tmp_bh, 1);
-		if (err)
-			goto out;
-		if (offset || tocopy != sb->s_blocksize)
-			bh = sb_bread(sb, tmp_bh.b_blocknr);
-		else
-			bh = sb_getblk(sb, tmp_bh.b_blocknr);
-		if (!bh) {
-			err = -EIO;
-			goto out;
+		pgoff_t index = pos / PAGE_SIZE;
+		size_t tocopy = min(PAGE_SIZE - offset_in_page(pos), towrite);
+		struct folio *folio;
+		void *dst;
+
+		if (offset_in_page(pos) ||
+		    (towrite < PAGE_SIZE && (pos + towrite < inode->i_size))) {
+			folio = read_mapping_folio(mapping, index, NULL);
+			if (IS_ERR(folio)) {
+				err = PTR_ERR(folio);
+				break;
+			}
+		} else {
+			folio = __filemap_get_folio(mapping, index,
+					FGP_CREAT|FGP_WRITE, GFP_KERNEL);
+			if (!folio) {
+				err = -ENOMEM;
+				break;
+			}
 		}
-		lock_buffer(bh);
-		memcpy(bh->b_data+offset, data, tocopy);
-		flush_dcache_page(bh->b_page);
-		set_buffer_uptodate(bh);
-		mark_buffer_dirty(bh);
-		unlock_buffer(bh);
-		brelse(bh);
-		offset = 0;
+
+		folio_lock(folio);
+		dst = kmap_local_folio(folio, offset_in_folio(folio, pos));
+		memcpy(dst, data, tocopy);
 		towrite -= tocopy;
 		data += tocopy;
-		blk++;
+		pos += tocopy;
+		if (!towrite && pos >= inode->i_size)
+			memset(dst + tocopy, 0, PAGE_SIZE - tocopy);
+		kunmap_local(dst);
+
+		folio_mark_uptodate(folio);
+		folio_mark_dirty(folio);
+		folio_unlock(folio);
+		folio_put(folio);
 	}
-out:
+
 	if (len == towrite) {
 		inode_unlock(inode);
 		return err;
 	}
-	if (inode->i_size < off+len-towrite)
-		i_size_write(inode, off+len-towrite);
+	if (inode->i_size < pos + len - towrite)
+		i_size_write(inode, pos + len - towrite);
 	inode->i_mtime = inode->i_ctime = current_time(inode);
 	mark_inode_dirty(inode);
 	inode_unlock(inode);
-- 
2.34.1

Re: [RFC PATCH 8/9] jfs: Write quota through the page cache

[Christoph Hellwig]

On Thu, May 26, 2022 at 08:29:09PM +0100, Matthew Wilcox (Oracle) wrote:
> +			folio = __filemap_get_folio(mapping, index,
> +					FGP_CREAT|FGP_WRITE, GFP_KERNEL);

missing spaces.

> +		folio_lock(folio);
> +		dst = kmap_local_folio(folio, offset_in_folio(folio, pos));
> +		memcpy(dst, data, tocopy);

mecpy_to_folio would be nice here.

And gain the helper seems generic, but unlike the read side modern
file system often want some kind of journaling.  Not sure if it is worth
sharing it just for ext2 and jfs.

[RFC PATCH 9/9] jfs: Convert buffered IO paths to iomap

[Matthew Wilcox (Oracle)]

Use the iomap helper functions and remove jfs_get_block().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/jfs/inode.c        | 70 ++++++++-----------------------------------
 fs/jfs/jfs_inode.h    |  1 -
 fs/jfs/jfs_logmgr.c   |  1 -
 fs/jfs/jfs_metapage.c |  1 -
 4 files changed, 12 insertions(+), 61 deletions(-)

diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
index 19c091d9c20e..f1d4c308e047 100644
--- a/fs/jfs/inode.c
+++ b/fs/jfs/inode.c
@@ -7,7 +7,6 @@
 #include <linux/fs.h>
 #include <linux/iomap.h>
 #include <linux/mpage.h>
-#include <linux/buffer_head.h>
 #include <linux/pagemap.h>
 #include <linux/quotaops.h>
 #include <linux/uio.h>
@@ -298,69 +297,25 @@ const struct iomap_ops jfs_iomap_ops = {
 	.iomap_begin =  jfs_iomap_begin,
 };
 
-int jfs_get_block(struct inode *ip, sector_t lblock,
-		  struct buffer_head *bh_result, int create)
-{
-	struct iomap iomap = { };
-	int ret;
-
-	ret = jfs_iomap_begin(ip, lblock << ip->i_blkbits, bh_result->b_size,
-			create ? IOMAP_WRITE : 0, &iomap, NULL);
-	if (ret)
-		return ret;
-
-	bh_result->b_size = iomap.length;
-	if (iomap.type == IOMAP_HOLE)
-		return 0;
-
-	map_bh(bh_result, ip->i_sb, iomap.addr >> ip->i_blkbits);
-	if (iomap.flags & IOMAP_F_NEW)
-		set_buffer_new(bh_result);
-	return 0;
-}
-
-static int jfs_writepage(struct page *page, struct writeback_control *wbc)
-{
-	return block_write_full_page(page, jfs_get_block, wbc);
-}
+static const struct iomap_writeback_ops jfs_writeback_ops = {
+};
 
 static int jfs_writepages(struct address_space *mapping,
 			struct writeback_control *wbc)
 {
-	return mpage_writepages(mapping, wbc, jfs_get_block);
+	struct iomap_writepage_ctx wpc = { };
+
+	return iomap_writepages(mapping, wbc, &wpc, &jfs_writeback_ops);
 }
 
 static int jfs_read_folio(struct file *file, struct folio *folio)
 {
-	return mpage_read_folio(folio, jfs_get_block);
+	return iomap_read_folio(folio, &jfs_iomap_ops);
 }
 
 static void jfs_readahead(struct readahead_control *rac)
 {
-	mpage_readahead(rac, jfs_get_block);
-}
-
-static void jfs_write_failed(struct address_space *mapping, loff_t to)
-{
-	struct inode *inode = mapping->host;
-
-	if (to > inode->i_size) {
-		truncate_pagecache(inode, inode->i_size);
-		jfs_truncate(inode);
-	}
-}
-
-static int jfs_write_begin(struct file *file, struct address_space *mapping,
-				loff_t pos, unsigned len,
-				struct page **pagep, void **fsdata)
-{
-	int ret;
-
-	ret = nobh_write_begin(mapping, pos, len, pagep, fsdata, jfs_get_block);
-	if (unlikely(ret))
-		jfs_write_failed(mapping, pos + len);
-
-	return ret;
+	iomap_readahead(rac, &jfs_iomap_ops);
 }
 
 static sector_t jfs_bmap(struct address_space *mapping, sector_t block)
@@ -369,14 +324,11 @@ static sector_t jfs_bmap(struct address_space *mapping, sector_t block)
 }
 
 const struct address_space_operations jfs_aops = {
-	.dirty_folio	= block_dirty_folio,
-	.invalidate_folio = block_invalidate_folio,
+	.dirty_folio	= filemap_dirty_folio,
+	.invalidate_folio = iomap_invalidate_folio,
 	.read_folio	= jfs_read_folio,
 	.readahead	= jfs_readahead,
-	.writepage	= jfs_writepage,
 	.writepages	= jfs_writepages,
-	.write_begin	= jfs_write_begin,
-	.write_end	= nobh_write_end,
 	.bmap		= jfs_bmap,
 	.direct_IO	= noop_direct_IO,
 };
@@ -427,9 +379,11 @@ void jfs_truncate_nolock(struct inode *ip, loff_t length)
 
 void jfs_truncate(struct inode *ip)
 {
+	bool did_zero;
+
 	jfs_info("jfs_truncate: size = 0x%lx", (ulong) ip->i_size);
 
-	nobh_truncate_page(ip->i_mapping, ip->i_size, jfs_get_block);
+	iomap_truncate_page(ip, i_size_read(ip), &did_zero, &jfs_iomap_ops);
 
 	IWRITE_LOCK(ip, RDWRLOCK_NORMAL);
 	jfs_truncate_nolock(ip, ip->i_size);
diff --git a/fs/jfs/jfs_inode.h b/fs/jfs/jfs_inode.h
index afd12de3c341..b1d3816a1a5c 100644
--- a/fs/jfs/jfs_inode.h
+++ b/fs/jfs/jfs_inode.h
@@ -27,7 +27,6 @@ extern struct dentry *jfs_fh_to_dentry(struct super_block *sb, struct fid *fid,
 extern struct dentry *jfs_fh_to_parent(struct super_block *sb, struct fid *fid,
 	int fh_len, int fh_type);
 extern void jfs_set_inode_flags(struct inode *);
-extern int jfs_get_block(struct inode *, sector_t, struct buffer_head *, int);
 extern int jfs_setattr(struct user_namespace *, struct dentry *, struct iattr *);
 
 extern const struct iomap_ops jfs_iomap_ops;
diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
index 997c81fcea34..466105331735 100644
--- a/fs/jfs/jfs_logmgr.c
+++ b/fs/jfs/jfs_logmgr.c
@@ -51,7 +51,6 @@
 #include <linux/interrupt.h>
 #include <linux/completion.h>
 #include <linux/kthread.h>
-#include <linux/buffer_head.h>		/* for sync_blockdev() */
 #include <linux/bio.h>
 #include <linux/freezer.h>
 #include <linux/export.h>
diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c
index 5b4f0cd8d276..dd9b855092fd 100644
--- a/fs/jfs/jfs_metapage.c
+++ b/fs/jfs/jfs_metapage.c
@@ -10,7 +10,6 @@
 #include <linux/bio.h>
 #include <linux/slab.h>
 #include <linux/init.h>
-#include <linux/buffer_head.h>
 #include <linux/mempool.h>
 #include <linux/seq_file.h>
 #include <linux/writeback.h>
-- 
2.34.1

Re: [RFC PATCH 0/9] Convert JFS to use iomap

[Dave Chinner]

On Thu, May 26, 2022 at 08:29:01PM +0100, Matthew Wilcox (Oracle) wrote:
> This patchset does not work.  It will eat your filesystem.  Do not apply.
> 
> The bug starts to show up with the fourth patch ("Convert direct_IO write
> support to use iomap").  generic/013 creates a corrupt filesystem and
> fsck fails to fix it, which shows all kinds of fun places in xfstests
> where we neglect to check that 'mount' actually mounted the filesystem.
> set -x or die.
> 
> I'm hoping one of the people who knows iomap better than I do can just
> point at the bug and say "Duh, it doesn't work like that".
> 
> It's safe to say that every patch after patch 6 is untested.  I'm not
> convinced that I really tested patch 6 either.

So the question I have to ask here is "why even bother?".

JFS is a legacy filesystem, and the risk of breaking stuff or
corrupting data and nobody noticing is really quite high.

We recently deprecated reiserfs and scheduled it's removal because
of the burden of keeping it up to date with VFS changes, what makes
JFS any different in this regard?

i.e. shouldn't this patchset be an indication that we should be
seriously considering deprecating and removing more legacy
filesystems rather than taking on the risk and burden associated
with updating them to modern internal kernel interfaces?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 0/9] Convert JFS to use iomap

[Matthew Wilcox]

On Sat, May 28, 2022 at 10:02:16AM +1000, Dave Chinner wrote:
> On Thu, May 26, 2022 at 08:29:01PM +0100, Matthew Wilcox (Oracle) wrote:
> > This patchset does not work.  It will eat your filesystem.  Do not apply.
> > 
> > The bug starts to show up with the fourth patch ("Convert direct_IO write
> > support to use iomap").  generic/013 creates a corrupt filesystem and
> > fsck fails to fix it, which shows all kinds of fun places in xfstests
> > where we neglect to check that 'mount' actually mounted the filesystem.
> > set -x or die.
> > 
> > I'm hoping one of the people who knows iomap better than I do can just
> > point at the bug and say "Duh, it doesn't work like that".
> > 
> > It's safe to say that every patch after patch 6 is untested.  I'm not
> > convinced that I really tested patch 6 either.
> 
> So the question I have to ask here is "why even bother?".
> 
> JFS is a legacy filesystem, and the risk of breaking stuff or
> corrupting data and nobody noticing is really quite high.
> 
> We recently deprecated reiserfs and scheduled it's removal because
> of the burden of keeping it up to date with VFS changes, what makes
> JFS any different in this regard?

Deprecating and scheduling removal is all well and good (and yes,
we should probably have a serious conversation about when we should
remove JFS), but JFS is one of the two users of the nobh infrastructure.
If we want to get rid of the nobh infrastructure (which I do), we need
to transition JFS to some other infrastructure.

We also need to convert more filesystems to use iomap.  I really wanted
to NAK the ntfs3 submission on the basis that it was still BH based,
but with so many existing filesystems using the BH infrastructure,
that's not a credible thing to require.

So JFS stood out to me as a filesystem which uses infrastructure that we
can remove fairly easily, one which doesn't get a whole lot of patches,
one that doesn't really use a lot of the BH infrastructure anyway and
one which can serve as an example for more ... relevant filesystems.

Re: [RFC PATCH 0/9] Convert JFS to use iomap

[Dave Chinner]

On Sat, May 28, 2022 at 03:15:57AM +0100, Matthew Wilcox wrote:
> On Sat, May 28, 2022 at 10:02:16AM +1000, Dave Chinner wrote:
> > On Thu, May 26, 2022 at 08:29:01PM +0100, Matthew Wilcox (Oracle) wrote:
> > > This patchset does not work.  It will eat your filesystem.  Do not apply.
> > > 
> > > The bug starts to show up with the fourth patch ("Convert direct_IO write
> > > support to use iomap").  generic/013 creates a corrupt filesystem and
> > > fsck fails to fix it, which shows all kinds of fun places in xfstests
> > > where we neglect to check that 'mount' actually mounted the filesystem.
> > > set -x or die.
> > > 
> > > I'm hoping one of the people who knows iomap better than I do can just
> > > point at the bug and say "Duh, it doesn't work like that".
> > > 
> > > It's safe to say that every patch after patch 6 is untested.  I'm not
> > > convinced that I really tested patch 6 either.
> > 
> > So the question I have to ask here is "why even bother?".
> > 
> > JFS is a legacy filesystem, and the risk of breaking stuff or
> > corrupting data and nobody noticing is really quite high.
> > 
> > We recently deprecated reiserfs and scheduled it's removal because
> > of the burden of keeping it up to date with VFS changes, what makes
> > JFS any different in this regard?
> 
> Deprecating and scheduling removal is all well and good (and yes,
> we should probably have a serious conversation about when we should
> remove JFS), but JFS is one of the two users of the nobh infrastructure.
> If we want to get rid of the nobh infrastructure (which I do), we need
> to transition JFS to some other infrastructure.

Sure, but ... Devil's Advocate.

The other filesystem that uses nobh is the standalone ext2
filesystem that nobody uses anymore as the ext4 module provides ext2
functionality for distros these days. Hence there's an argument that
can be made for removing fs/ext2 as well. In which case, the whole
nobh problem goes away by deprecating and removing both the
filesysetms that use that infrastructure in 2 years time....

> We also need to convert more filesystems to use iomap.

We also need to deprecate and remove more largely unmaintained and
unused filesystems. :)

> I really wanted
> to NAK the ntfs3 submission on the basis that it was still BH based,
> but with so many existing filesystems using the BH infrastructure,
> that's not a credible thing to require.

Until ext4 is converted to use iomap, we realistically cannot ask
anyone to use iomap....

> So JFS stood out to me as a filesystem which uses infrastructure that we
> can remove fairly easily, one which doesn't get a whole lot of patches,
> one that doesn't really use a lot of the BH infrastructure anyway and
> one which can serve as an example for more ... relevant filesystems.

Isn't that the entire purpose of fs/ext2 still existing these days?
i.e. to be the simple "reference" filesystem for Linux?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 0/9] Convert JFS to use iomap

[Theodore Ts'o]

+linux-ext4

On Sat, May 28, 2022 at 03:36:39PM +1000, Dave Chinner wrote:
> The other filesystem that uses nobh is the standalone ext2
> filesystem that nobody uses anymore as the ext4 module provides ext2
> functionality for distros these days. Hence there's an argument that
> can be made for removing fs/ext2 as well. In which case, the whole
> nobh problem goes away by deprecating and removing both the
> filesysetms that use that infrastructure in 2 years time....

This got brought up at this past week's ext4 video chat, where Willy
asked Jan (who has been maintaining ext2) whether he would be open to
converting ext2 to use iomap.  The answer was yes.  So once jfs and
ext2 are converted, we'll be able to nuke the nobh code.

From Willy's comments on the video chat, my understanding is that jfs
was even simpler to convert that ext2, and this allows us to remove
the nobh infrastructure without asking the question about whether it's
time to remove jfs.

> > We also need to convert more filesystems to use iomap.
> 
> We also need to deprecate and remove more largely unmaintained and
> unused filesystems. :)

Well, Dave Kleikamp is still around and sends jfs pull requests from
time to time, and so it's not as unmaintained as, say, fs/adfs,
fs/freevxs, fs/hpfs, fs/minix, and fs/sysv.

As regards to minixfs, I'd argue that ext2 is a better reference file
system than minixfs.  So..... are we ready to remove minixfs?  I could
easily see that some folks might still have sentimental attachment to
minixfs.  :-)

> Until ext4 is converted to use iomap, we realistically cannot ask
> anyone to use iomap....

That's something that we've been discussing on the ext4 video chats.
What we can probably do is to convert buffered I/O to use iomap in
stages, first starting with the easy case, and then progressing to the
more complex ones:

     * page_size == block_size, !fscrypt, !fsverity, !data=journal
     * page_size != block_size, !fscrypt, !fsverity, !data=journal
     * fsverity
     * fscrypt

At that point, the hard, remaining case is what to do with
data=journal.  data=journal is already barely maintained; we don't
support direct i/o, delayed allocation, etc., There have been some
specialized users for it, but it's probably more for interesting
research applications than anything else.

So the question is whether we keep it as a special case, and never
convert it over to iomap, or decide that it's time to deprecate and
rip out data=journal support.  We don't need to make that decision
right away, and so long as it remains a special case where it doesn't
burden the rest of the kernel, we might end up keeping it so long as
it remains a minimal maintenance burden for ext4.  I duuno....

In any case, rest assured that there have been quite a lot of
discussions about how to convert all (or 99.99%) of ext4 to use iomap.

Cheers,

						- Ted

Re: [RFC PATCH 0/9] Convert JFS to use iomap

[Dave Chinner]

On Sat, May 28, 2022 at 02:59:31PM -0400, Theodore Ts'o wrote:
> +linux-ext4
> 
> On Sat, May 28, 2022 at 03:36:39PM +1000, Dave Chinner wrote:
> > The other filesystem that uses nobh is the standalone ext2
> > filesystem that nobody uses anymore as the ext4 module provides ext2
> > functionality for distros these days. Hence there's an argument that
> > can be made for removing fs/ext2 as well. In which case, the whole
> > nobh problem goes away by deprecating and removing both the
> > filesysetms that use that infrastructure in 2 years time....
> 
> This got brought up at this past week's ext4 video chat, where Willy
> asked Jan (who has been maintaining ext2) whether he would be open to
> converting ext2 to use iomap.  The answer was yes.  So once jfs and
> ext2 are converted, we'll be able to nuke the nobh code.
> 
> From Willy's comments on the video chat, my understanding is that jfs
> was even simpler to convert that ext2, and this allows us to remove
> the nobh infrastructure without asking the question about whether it's
> time to remove jfs.

I disagree there - if we are changing code that has been unchanged
for a decade or more, there are very few users of that code, and
there's a good chance that data corruption regressions will result
from the changes being proposed, then asking the question "why take
the risk" is very pertinent.

"Just because we can" isn't a good answer. The best code is code we
don't have to write and maintain. If it's a burden to maintain and a
barrier to progress, then we should seriously be considering
removing it, not trying to maintain the fiction that it's a viable
supported production quality filesystem that people can rely on....

> > > We also need to convert more filesystems to use iomap.
> > 
> > We also need to deprecate and remove more largely unmaintained and
> > unused filesystems. :)
> 
> Well, Dave Kleikamp is still around and sends jfs pull requests from
> time to time, and so it's not as unmaintained as, say, fs/adfs,
> fs/freevxs, fs/hpfs, fs/minix, and fs/sysv.

Yes, but the changes that have been made over the past decade are
all small and minor - there's been no feature work, no cleanup work,
no attempt to update core infrastructure, etc. There's beeen no
serious attempts to modernise or update the code for a decade...

> As regards to minixfs, I'd argue that ext2 is a better reference file
> system than minixfs.  So..... are we ready to remove minixfs?  I could
> easily see that some folks might still have sentimental attachment to
> minixfs.  :-)

AFAIC, yes, minixfs and and those other ones should have been
deprecated long ago....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [Jfs-discussion] [RFC PATCH 0/9] Convert JFS to use iomap

[Dave Kleikamp]

On 5/29/22 6:51PM, Dave Chinner wrote:
> On Sat, May 28, 2022 at 02:59:31PM -0400, Theodore Ts'o wrote:
>> +linux-ext4
>>
>> On Sat, May 28, 2022 at 03:36:39PM +1000, Dave Chinner wrote:
>>> The other filesystem that uses nobh is the standalone ext2
>>> filesystem that nobody uses anymore as the ext4 module provides ext2
>>> functionality for distros these days. Hence there's an argument that
>>> can be made for removing fs/ext2 as well. In which case, the whole
>>> nobh problem goes away by deprecating and removing both the
>>> filesysetms that use that infrastructure in 2 years time....
>>
>> This got brought up at this past week's ext4 video chat, where Willy
>> asked Jan (who has been maintaining ext2) whether he would be open to
>> converting ext2 to use iomap.  The answer was yes.  So once jfs and
>> ext2 are converted, we'll be able to nuke the nobh code.
>>
>>  From Willy's comments on the video chat, my understanding is that jfs
>> was even simpler to convert that ext2, and this allows us to remove
>> the nobh infrastructure without asking the question about whether it's
>> time to remove jfs.
> 
> I disagree there - if we are changing code that has been unchanged
> for a decade or more, there are very few users of that code, and
> there's a good chance that data corruption regressions will result
> from the changes being proposed, then asking the question "why take
> the risk" is very pertinent.
> 
> "Just because we can" isn't a good answer. The best code is code we
> don't have to write and maintain. If it's a burden to maintain and a
> barrier to progress, then we should seriously be considering
> removing it, not trying to maintain the fiction that it's a viable
> supported production quality filesystem that people can rely on....

I'm onboard to sunsetting jfs. I don't know of anyone that is currently 
using it in any serious way. (jfs-discussion group, this is a good time 
to chime in if you feel differently.)

On the other hand, because it is not being used in an any 
mission-critical way, it may be a good filesystem to do an early 
conversion on to see what issues might come up. Unfortunately, I've got 
a really busy two months in front of me and won't be much help.

Thanks,
Shaggy

> 
>>>> We also need to convert more filesystems to use iomap.
>>>
>>> We also need to deprecate and remove more largely unmaintained and
>>> unused filesystems. :)
>>
>> Well, Dave Kleikamp is still around and sends jfs pull requests from
>> time to time, and so it's not as unmaintained as, say, fs/adfs,
>> fs/freevxs, fs/hpfs, fs/minix, and fs/sysv.
> 
> Yes, but the changes that have been made over the past decade are
> all small and minor - there's been no feature work, no cleanup work,
> no attempt to update core infrastructure, etc. There's beeen no
> serious attempts to modernise or update the code for a decade...
> 
>> As regards to minixfs, I'd argue that ext2 is a better reference file
>> system than minixfs.  So..... are we ready to remove minixfs?  I could
>> easily see that some folks might still have sentimental attachment to
>> minixfs.  :-)
> 
> AFAIC, yes, minixfs and and those other ones should have been
> deprecated long ago....
> 
> Cheers,
> 
> Dave.

Re: [Jfs-discussion] [RFC PATCH 0/9] Convert JFS to use iomap

[Matthew Wilcox]

On Tue, May 31, 2022 at 08:51:40AM -0500, Dave Kleikamp wrote:
> On 5/29/22 6:51PM, Dave Chinner wrote:
> > "Just because we can" isn't a good answer. The best code is code we
> > don't have to write and maintain. If it's a burden to maintain and a
> > barrier to progress, then we should seriously be considering
> > removing it, not trying to maintain the fiction that it's a viable
> > supported production quality filesystem that people can rely on....
> 
> I'm onboard to sunsetting jfs. I don't know of anyone that is currently
> using it in any serious way. (jfs-discussion group, this is a good time to
> chime in if you feel differently.)

We should probably get the mythtv people to stop recommending JFS.

https://www.mythtv.org/wiki/User_Manual:Setting_Up#Filesystems

Re: [Jfs-discussion] [RFC PATCH 0/9] Convert JFS to use iomap

[Dave Kleikamp]

On 5/31/22 10:31AM, Matthew Wilcox wrote:
> On Tue, May 31, 2022 at 08:51:40AM -0500, Dave Kleikamp wrote:
>> On 5/29/22 6:51PM, Dave Chinner wrote:
>>> "Just because we can" isn't a good answer. The best code is code we
>>> don't have to write and maintain. If it's a burden to maintain and a
>>> barrier to progress, then we should seriously be considering
>>> removing it, not trying to maintain the fiction that it's a viable
>>> supported production quality filesystem that people can rely on....
>>
>> I'm onboard to sunsetting jfs. I don't know of anyone that is currently
>> using it in any serious way. (jfs-discussion group, this is a good time to
>> chime in if you feel differently.)
> 
> We should probably get the mythtv people to stop recommending JFS.
> 
> https://www.mythtv.org/wiki/User_Manual:Setting_Up#Filesystems

Good point. Maybe not mission-critical, but introducing any new 
potential file corruption can sure make for some unhappy people.