diff mbox

[4/6] xfs: Set BH_New for allocated DAX blocks in __xfs_get_blocks()

Message ID 1474994615-29553-5-git-send-email-jack@suse.cz (mailing list archive)
State Not Applicable
Headers show

Commit Message

Jan Kara Sept. 27, 2016, 4:43 p.m. UTC
So far we did not set BH_New for newly allocated blocks for DAX inodes
in __xfs_get_blocks() because we wanted to avoid zeroing done in generic
DAX code which was racy. Now the zeroing is gone so we can remove this
workaround and return BH_New for newly allocated blocks. DAX will use this
information to properly update mappings of the file.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_aops.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Christoph Hellwig Sept. 27, 2016, 5:01 p.m. UTC | #1
On Tue, Sep 27, 2016 at 06:43:33PM +0200, Jan Kara wrote:
> So far we did not set BH_New for newly allocated blocks for DAX inodes
> in __xfs_get_blocks() because we wanted to avoid zeroing done in generic
> DAX code which was racy. Now the zeroing is gone so we can remove this
> workaround and return BH_New for newly allocated blocks. DAX will use this
> information to properly update mappings of the file.

__xfs_get_blocks isn't used by the DAX code any more.
xfs_file_iomap_begin should already be doing the right thing for now.
Jan Kara Sept. 27, 2016, 5:17 p.m. UTC | #2
On Tue 27-09-16 10:01:18, Christoph Hellwig wrote:
> On Tue, Sep 27, 2016 at 06:43:33PM +0200, Jan Kara wrote:
> > So far we did not set BH_New for newly allocated blocks for DAX inodes
> > in __xfs_get_blocks() because we wanted to avoid zeroing done in generic
> > DAX code which was racy. Now the zeroing is gone so we can remove this
> > workaround and return BH_New for newly allocated blocks. DAX will use this
> > information to properly update mappings of the file.
> 
> __xfs_get_blocks isn't used by the DAX code any more.
> xfs_file_iomap_begin should already be doing the right thing for now.

OK, the changelog is stale but I actually took care to integrate this with
your iomap patches and for the new invalidation code in iomap_dax_actor()
to work we need this additional information...

								Honza
Dave Chinner Sept. 28, 2016, 12:22 a.m. UTC | #3
On Tue, Sep 27, 2016 at 07:17:07PM +0200, Jan Kara wrote:
> On Tue 27-09-16 10:01:18, Christoph Hellwig wrote:
> > On Tue, Sep 27, 2016 at 06:43:33PM +0200, Jan Kara wrote:
> > > So far we did not set BH_New for newly allocated blocks for DAX inodes
> > > in __xfs_get_blocks() because we wanted to avoid zeroing done in generic
> > > DAX code which was racy. Now the zeroing is gone so we can remove this
> > > workaround and return BH_New for newly allocated blocks. DAX will use this
> > > information to properly update mappings of the file.
> > 
> > __xfs_get_blocks isn't used by the DAX code any more.
> > xfs_file_iomap_begin should already be doing the right thing for now.
> 
> OK, the changelog is stale but I actually took care to integrate this with
> your iomap patches and for the new invalidation code in iomap_dax_actor()
> to work we need this additional information...

So this applies to the iomap-4.9-dax branch in the XFS tree?

https://git.kernel.org/cgit/linux/kernel/git/dgc/linux-xfs.git/log/?h=iomap-4.9-dax

i.e. Do I need to merge the patchset with that branch?

Cheers,

Dave.
Christoph Hellwig Sept. 28, 2016, 2:13 a.m. UTC | #4
On Tue, Sep 27, 2016 at 07:17:07PM +0200, Jan Kara wrote:
> OK, the changelog is stale but I actually took care to integrate this with
> your iomap patches and for the new invalidation code in iomap_dax_actor()
> to work we need this additional information...

It's not just the changelogs (which will need updates on more than this
patch), but also the content.  We're not using get_blocks for DAX
anymore, so this patch should not be needed anymore.
Jan Kara Oct. 3, 2016, 2:42 p.m. UTC | #5
On Tue 27-09-16 19:13:50, Christoph Hellwig wrote:
> On Tue, Sep 27, 2016 at 07:17:07PM +0200, Jan Kara wrote:
> > OK, the changelog is stale but I actually took care to integrate this with
> > your iomap patches and for the new invalidation code in iomap_dax_actor()
> > to work we need this additional information...
> 
> It's not just the changelogs (which will need updates on more than this
> patch), but also the content.  We're not using get_blocks for DAX
> anymore, so this patch should not be needed anymore.

Ah, you are right. After reading the code again this patch is not needed
anymore because xfs_file_iomap_begin() returns IOMAP_F_NEW whenever we
allocated block regardless whether it was zeroed-out or not. But if I get it
right, all the IS_DAX checks in __xfs_get_blocks() can be dropped now, cannot
they?

								Honza
Jan Kara Oct. 3, 2016, 2:44 p.m. UTC | #6
On Wed 28-09-16 10:22:55, Dave Chinner wrote:
> On Tue, Sep 27, 2016 at 07:17:07PM +0200, Jan Kara wrote:
> > On Tue 27-09-16 10:01:18, Christoph Hellwig wrote:
> > > On Tue, Sep 27, 2016 at 06:43:33PM +0200, Jan Kara wrote:
> > > > So far we did not set BH_New for newly allocated blocks for DAX inodes
> > > > in __xfs_get_blocks() because we wanted to avoid zeroing done in generic
> > > > DAX code which was racy. Now the zeroing is gone so we can remove this
> > > > workaround and return BH_New for newly allocated blocks. DAX will use this
> > > > information to properly update mappings of the file.
> > > 
> > > __xfs_get_blocks isn't used by the DAX code any more.
> > > xfs_file_iomap_begin should already be doing the right thing for now.
> > 
> > OK, the changelog is stale but I actually took care to integrate this with
> > your iomap patches and for the new invalidation code in iomap_dax_actor()
> > to work we need this additional information...
> 
> So this applies to the iomap-4.9-dax branch in the XFS tree?

Yes.

> https://git.kernel.org/cgit/linux/kernel/git/dgc/linux-xfs.git/log/?h=iomap-4.9-dax
> 
> i.e. Do I need to merge the patchset with that branch?

This patch series cannot be immediately applied as it functionally depends
on the patches to allow safe clearing of dirty bits in DAX mappings.

								Honza
Christoph Hellwig Oct. 3, 2016, 4:39 p.m. UTC | #7
On Mon, Oct 03, 2016 at 04:42:46PM +0200, Jan Kara wrote:
> Ah, you are right. After reading the code again this patch is not needed
> anymore because xfs_file_iomap_begin() returns IOMAP_F_NEW whenever we
> allocated block regardless whether it was zeroed-out or not. But if I get it
> right, all the IS_DAX checks in __xfs_get_blocks() can be dropped now, cannot
> they?

Right now the code is required to keep the pmd_fault handler (which
is always disabled though) compiling.  Once that one is gone the
DAX code in __xfs_get_blocks can be removed.  In fact Ross sent a
patch to do just that as part of his PMD fault rework.
diff mbox

Patch

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 4a28fa91e3b1..b25b7b5a1e6e 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -1245,11 +1245,8 @@  __xfs_get_blocks(
 		goto out_unlock;
 	}
 
-	if (IS_DAX(inode) && create) {
+	if (IS_DAX(inode) && create)
 		ASSERT(!ISUNWRITTEN(&imap));
-		/* zeroing is not needed at a higher layer */
-		new = 0;
-	}
 
 	/* trim mapping down to size requested */
 	xfs_map_trim_size(inode, iblock, bh_result, &imap, offset, size);