diff mbox series

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

Message ID 20220526192910.357055-3-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series Convert JFS to use iomap | expand

Commit Message

Matthew Wilcox May 26, 2022, 7:29 p.m. UTC 
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(-)

Comments

Christoph Hellwig May 27, 2022, 5:41 a.m. UTC | #1 
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.
Matthew Wilcox May 27, 2022, 1:45 p.m. UTC | #2 
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.
Dave Kleikamp May 27, 2022, 2:58 p.m. UTC | #3 
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
diff mbox series

Patch

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