diff mbox

[v5,3/5] dax: use sb_issue_zerout instead of calling dax_clear_sectors

Message ID 1462571591-3361-4-git-send-email-vishal.l.verma@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Verma, Vishal L May 6, 2016, 9:53 p.m. UTC
From: Matthew Wilcox <matthew.r.wilcox@intel.com>

dax_clear_sectors() cannot handle poisoned blocks.  These must be
zeroed using the BIO interface instead.  Convert ext2 and XFS to use
only sb_issue_zerout().

Signed-off-by: Matthew Wilcox <matthew.r.wilcox@intel.com>
[vishal: Also remove the dax_clear_sectors function entirely]
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 fs/dax.c               | 32 --------------------------------
 fs/ext2/inode.c        |  7 +++----
 fs/xfs/xfs_bmap_util.c | 15 ++++-----------
 include/linux/dax.h    |  1 -
 4 files changed, 7 insertions(+), 48 deletions(-)

Comments

Christoph Hellwig May 8, 2016, 8:52 a.m. UTC | #1
On Fri, May 06, 2016 at 03:53:09PM -0600, Vishal Verma wrote:
> From: Matthew Wilcox <matthew.r.wilcox@intel.com>
> 
> dax_clear_sectors() cannot handle poisoned blocks.  These must be
> zeroed using the BIO interface instead.  Convert ext2 and XFS to use
> only sb_issue_zerout().
> 
> Signed-off-by: Matthew Wilcox <matthew.r.wilcox@intel.com>
> [vishal: Also remove the dax_clear_sectors function entirely]
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Just to make sure:  the existing sb_issue_zerout as in 4.6-rc
is already doing the right thing for DAX?  I've got a pending patchset
for XFS that introduces another dax_clear_sectors users, but if it's
already safe to use blkdev_issue_zeroout I can switch to that and avoid
the merge conflict.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Verma, Vishal L May 8, 2016, 6:46 p.m. UTC | #2
On Sun, 2016-05-08 at 01:52 -0700, Christoph Hellwig wrote:
> On Fri, May 06, 2016 at 03:53:09PM -0600, Vishal Verma wrote:

> > 

> > From: Matthew Wilcox <matthew.r.wilcox@intel.com>

> > 

> > dax_clear_sectors() cannot handle poisoned blocks.  These must be

> > zeroed using the BIO interface instead.  Convert ext2 and XFS to

> > use

> > only sb_issue_zerout().

> > 

> > Signed-off-by: Matthew Wilcox <matthew.r.wilcox@intel.com>

> > [vishal: Also remove the dax_clear_sectors function entirely]

> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

> Just to make sure:  the existing sb_issue_zerout as in 4.6-rc

> is already doing the right thing for DAX?  I've got a pending

> patchset

> for XFS that introduces another dax_clear_sectors users, but if it's

> already safe to use blkdev_issue_zeroout I can switch to that and

> avoid

> the merge conflict.


I believe so - Jan has moved all unwritten extent conversions out of
DAX with his patch set, and I believe zeroing through the driver is
always fine. Ross or Jan could confirm though.
Ross Zwisler May 9, 2016, 2:55 p.m. UTC | #3
On Sun, May 08, 2016 at 06:46:13PM +0000, Verma, Vishal L wrote:
> On Sun, 2016-05-08 at 01:52 -0700, Christoph Hellwig wrote:
> > On Fri, May 06, 2016 at 03:53:09PM -0600, Vishal Verma wrote:
> > > 
> > > From: Matthew Wilcox <matthew.r.wilcox@intel.com>
> > > 
> > > dax_clear_sectors() cannot handle poisoned blocks.  These must be
> > > zeroed using the BIO interface instead.  Convert ext2 and XFS to
> > > use
> > > only sb_issue_zerout().
> > > 
> > > Signed-off-by: Matthew Wilcox <matthew.r.wilcox@intel.com>
> > > [vishal: Also remove the dax_clear_sectors function entirely]
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > Just to make sure:  the existing sb_issue_zerout as in 4.6-rc
> > is already doing the right thing for DAX?  I've got a pending
> > patchset
> > for XFS that introduces another dax_clear_sectors users, but if it's
> > already safe to use blkdev_issue_zeroout I can switch to that and
> > avoid
> > the merge conflict.
> 
> I believe so - Jan has moved all unwritten extent conversions out of
> DAX with his patch set, and I believe zeroing through the driver is
> always fine. Ross or Jan could confirm though. 

Yep, I believe that the existing sb_issue_zeroout() as of v4.6-rc* does the
right thing.  We'll end up calling sb_issue_zeroout() => blkdev_issue_zeroout()
=> __blkdev_issue_zeroout() because we don't have support for discard or
write_same in PMEM.  This will send zero page BIOs to the PMEM driver, which
will do the zeroing as normal writes.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara May 10, 2016, 2:16 p.m. UTC | #4
On Fri 06-05-16 15:53:09, Vishal Verma wrote:
> From: Matthew Wilcox <matthew.r.wilcox@intel.com>
> 
> dax_clear_sectors() cannot handle poisoned blocks.  These must be
> zeroed using the BIO interface instead.  Convert ext2 and XFS to use
> only sb_issue_zerout().
> 
> Signed-off-by: Matthew Wilcox <matthew.r.wilcox@intel.com>
> [vishal: Also remove the dax_clear_sectors function entirely]
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

The patch looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
diff mbox

Patch

diff --git a/fs/dax.c b/fs/dax.c
index 52f0044..5948d9b 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -116,38 +116,6 @@  struct page *read_dax_sector(struct block_device *bdev, sector_t n)
 	return page;
 }
 
-/*
- * dax_clear_sectors() is called from within transaction context from XFS,
- * and hence this means the stack from this point must follow GFP_NOFS
- * semantics for all operations.
- */
-int dax_clear_sectors(struct block_device *bdev, sector_t _sector, long _size)
-{
-	struct blk_dax_ctl dax = {
-		.sector = _sector,
-		.size = _size,
-	};
-
-	might_sleep();
-	do {
-		long count, sz;
-
-		count = dax_map_atomic(bdev, &dax);
-		if (count < 0)
-			return count;
-		sz = min_t(long, count, SZ_128K);
-		clear_pmem(dax.addr, sz);
-		dax.size -= sz;
-		dax.sector += sz / 512;
-		dax_unmap_atomic(bdev, &dax);
-		cond_resched();
-	} while (dax.size);
-
-	wmb_pmem();
-	return 0;
-}
-EXPORT_SYMBOL_GPL(dax_clear_sectors);
-
 static bool buffer_written(struct buffer_head *bh)
 {
 	return buffer_mapped(bh) && !buffer_unwritten(bh);
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 1f07b75..35f2b0bf 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -26,6 +26,7 @@ 
 #include <linux/highuid.h>
 #include <linux/pagemap.h>
 #include <linux/dax.h>
+#include <linux/blkdev.h>
 #include <linux/quotaops.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
@@ -737,10 +738,8 @@  static int ext2_get_blocks(struct inode *inode,
 		 * so that it's not found by another thread before it's
 		 * initialised
 		 */
-		err = dax_clear_sectors(inode->i_sb->s_bdev,
-				le32_to_cpu(chain[depth-1].key) <<
-				(inode->i_blkbits - 9),
-				1 << inode->i_blkbits);
+		err = sb_issue_zeroout(inode->i_sb,
+				le32_to_cpu(chain[depth-1].key), 1, GFP_NOFS);
 		if (err) {
 			mutex_unlock(&ei->truncate_mutex);
 			goto cleanup;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 3b63098..930ac6a 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -72,18 +72,11 @@  xfs_zero_extent(
 	struct xfs_mount *mp = ip->i_mount;
 	xfs_daddr_t	sector = xfs_fsb_to_db(ip, start_fsb);
 	sector_t	block = XFS_BB_TO_FSBT(mp, sector);
-	ssize_t		size = XFS_FSB_TO_B(mp, count_fsb);
-
-	if (IS_DAX(VFS_I(ip)))
-		return dax_clear_sectors(xfs_find_bdev_for_inode(VFS_I(ip)),
-				sector, size);
-
-	/*
-	 * let the block layer decide on the fastest method of
-	 * implementing the zeroing.
-	 */
-	return sb_issue_zeroout(mp->m_super, block, count_fsb, GFP_NOFS);
 
+	return blkdev_issue_zeroout(xfs_find_bdev_for_inode(VFS_I(ip)),
+		block << (mp->m_super->s_blocksize_bits - 9),
+		count_fsb << (mp->m_super->s_blocksize_bits - 9),
+		GFP_NOFS, true);
 }
 
 /*
diff --git a/include/linux/dax.h b/include/linux/dax.h
index ef94fa7..426841a 100644
--- a/include/linux/dax.h
+++ b/include/linux/dax.h
@@ -11,7 +11,6 @@ 
 
 ssize_t dax_do_io(struct kiocb *, struct inode *, struct iov_iter *, loff_t,
 		  get_block_t, dio_iodone_t, int flags);
-int dax_clear_sectors(struct block_device *bdev, sector_t _sector, long _size);
 int dax_zero_page_range(struct inode *, loff_t from, unsigned len, get_block_t);
 int dax_truncate_page(struct inode *, loff_t from, get_block_t);
 int dax_fault(struct vm_area_struct *, struct vm_fault *, get_block_t);