| Message ID | 1474994615-29553-5-git-send-email-jack@suse.cz (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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.
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
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.
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.
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
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
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 --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);
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(-)