From: Ross Zwisler <ross.zwisler@linux.intel.com> To: Jan Kara <jack@suse.cz> Cc: linux-nvdimm@lists.01.org, NeilBrown <neilb@suse.com>, Wilcox, Subject: Re: [PATCH 04/10] dax: Fix data corruption for written and mmapped files Date: Wed, 23 Mar 2016 11:39:45 -0600 [thread overview] Message-ID: <20160323173945.GC5544@linux.intel.com> (raw) In-Reply-To: <1458566575-28063-5-git-send-email-jack@suse.cz> On Mon, Mar 21, 2016 at 02:22:49PM +0100, Jan Kara wrote: > When a fault to a hole races with write filling the hole, it can happen > that block zeroing in __dax_fault() overwrites the data copied by write. > Since filesystem is supposed to provide pre-zeroed blocks for fault > anyway, just remove the racy zeroing from dax code. The only catch is > with read-faults over unwritten block where __dax_fault() filled in the > block into page tables anyway. For that case we have to fall back to > using hole page now. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/dax.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index d496466652cd..50d81172438b 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -582,11 +582,6 @@ static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh, > error = PTR_ERR(dax.addr); > goto out; > } > - > - if (buffer_unwritten(bh) || buffer_new(bh)) { > - clear_pmem(dax.addr, PAGE_SIZE); > - wmb_pmem(); > - } I agree that we should be dropping these bits of code, but I think they are just dead code that could never be executed? I don't see how we could have hit a race? For the above, dax_insert_mapping() is only called if we actually have a block mapping (holes go through dax_load_hole()), so for ext4 and XFS I think buffer_unwritten() and buffer_new() are always false, so this code could never be executed, right? I suppose that maybe we could get into here via ext2 if BH_New was set? Is that the race? > dax_unmap_atomic(bdev, &dax); > > error = dax_radix_entry(mapping, vmf->pgoff, dax.sector, false, > @@ -665,7 +660,7 @@ int __dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf, > if (error) > goto unlock_page; > > - if (!buffer_mapped(&bh) && !buffer_unwritten(&bh) && !vmf->cow_page) { > + if (!buffer_mapped(&bh) && !vmf->cow_page) { Sure. > if (vmf->flags & FAULT_FLAG_WRITE) { > error = get_block(inode, block, &bh, 1); > count_vm_event(PGMAJFAULT); > @@ -950,8 +945,6 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address, > } > > if (buffer_unwritten(&bh) || buffer_new(&bh)) { > - clear_pmem(dax.addr, PMD_SIZE); > - wmb_pmem(); > count_vm_event(PGMAJFAULT); > mem_cgroup_count_vm_event(vma->vm_mm, PGMAJFAULT); > result |= VM_FAULT_MAJOR; I think this whole block is just dead code, right? Can we ever get into here? Same argument applies as from dax_insert_mapping() - if we get this far then we have a mapped buffer, and in the PMD case we know we're on ext4 of XFS since ext2 doesn't do huge page mappings. So, buffer_unwritten() and buffer_new() both always return false, right? Yea...we really need to clean up our buffer flag handling. :) _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
WARNING: multiple messages have this Message-ID (diff)
From: Ross Zwisler <ross.zwisler@linux.intel.com> To: Jan Kara <jack@suse.cz> Cc: linux-fsdevel@vger.kernel.org, "Wilcox, Matthew R" <matthew.r.wilcox@intel.com>, Ross Zwisler <ross.zwisler@linux.intel.com>, Dan Williams <dan.j.williams@intel.com>, linux-nvdimm@lists.01.org, NeilBrown <neilb@suse.com> Subject: Re: [PATCH 04/10] dax: Fix data corruption for written and mmapped files Date: Wed, 23 Mar 2016 11:39:45 -0600 [thread overview] Message-ID: <20160323173945.GC5544@linux.intel.com> (raw) In-Reply-To: <1458566575-28063-5-git-send-email-jack@suse.cz> On Mon, Mar 21, 2016 at 02:22:49PM +0100, Jan Kara wrote: > When a fault to a hole races with write filling the hole, it can happen > that block zeroing in __dax_fault() overwrites the data copied by write. > Since filesystem is supposed to provide pre-zeroed blocks for fault > anyway, just remove the racy zeroing from dax code. The only catch is > with read-faults over unwritten block where __dax_fault() filled in the > block into page tables anyway. For that case we have to fall back to > using hole page now. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/dax.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index d496466652cd..50d81172438b 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -582,11 +582,6 @@ static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh, > error = PTR_ERR(dax.addr); > goto out; > } > - > - if (buffer_unwritten(bh) || buffer_new(bh)) { > - clear_pmem(dax.addr, PAGE_SIZE); > - wmb_pmem(); > - } I agree that we should be dropping these bits of code, but I think they are just dead code that could never be executed? I don't see how we could have hit a race? For the above, dax_insert_mapping() is only called if we actually have a block mapping (holes go through dax_load_hole()), so for ext4 and XFS I think buffer_unwritten() and buffer_new() are always false, so this code could never be executed, right? I suppose that maybe we could get into here via ext2 if BH_New was set? Is that the race? > dax_unmap_atomic(bdev, &dax); > > error = dax_radix_entry(mapping, vmf->pgoff, dax.sector, false, > @@ -665,7 +660,7 @@ int __dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf, > if (error) > goto unlock_page; > > - if (!buffer_mapped(&bh) && !buffer_unwritten(&bh) && !vmf->cow_page) { > + if (!buffer_mapped(&bh) && !vmf->cow_page) { Sure. > if (vmf->flags & FAULT_FLAG_WRITE) { > error = get_block(inode, block, &bh, 1); > count_vm_event(PGMAJFAULT); > @@ -950,8 +945,6 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address, > } > > if (buffer_unwritten(&bh) || buffer_new(&bh)) { > - clear_pmem(dax.addr, PMD_SIZE); > - wmb_pmem(); > count_vm_event(PGMAJFAULT); > mem_cgroup_count_vm_event(vma->vm_mm, PGMAJFAULT); > result |= VM_FAULT_MAJOR; I think this whole block is just dead code, right? Can we ever get into here? Same argument applies as from dax_insert_mapping() - if we get this far then we have a mapped buffer, and in the PMD case we know we're on ext4 of XFS since ext2 doesn't do huge page mappings. So, buffer_unwritten() and buffer_new() both always return false, right? Yea...we really need to clean up our buffer flag handling. :)
next prev parent reply other threads:[~2016-03-23 17:40 UTC|newest] Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-03-21 13:22 [RFC v2] [PATCH 0/10] DAX page fault locking Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-21 13:22 ` [PATCH 01/10] DAX: move RADIX_DAX_ definitions to dax.c Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-21 17:25 ` Matthew Wilcox 2016-03-21 17:25 ` Matthew Wilcox 2016-03-21 13:22 ` [PATCH 02/10] radix-tree: make 'indirect' bit available to exception entries Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-21 17:34 ` Matthew Wilcox 2016-03-21 17:34 ` Matthew Wilcox 2016-03-22 9:12 ` Jan Kara 2016-03-22 9:12 ` Jan Kara 2016-03-22 9:27 ` Matthew Wilcox 2016-03-22 9:27 ` Matthew Wilcox 2016-03-22 10:37 ` Jan Kara 2016-03-22 10:37 ` Jan Kara 2016-03-23 16:41 ` Ross Zwisler 2016-03-23 16:41 ` Ross Zwisler 2016-03-24 12:31 ` Jan Kara 2016-03-24 12:31 ` Jan Kara 2016-03-21 13:22 ` [PATCH 03/10] dax: Remove complete_unwritten argument Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-23 17:12 ` Ross Zwisler 2016-03-23 17:12 ` Ross Zwisler 2016-03-24 12:32 ` Jan Kara 2016-03-24 12:32 ` Jan Kara 2016-03-21 13:22 ` [PATCH 04/10] dax: Fix data corruption for written and mmapped files Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-23 17:39 ` Ross Zwisler [this message] 2016-03-23 17:39 ` Ross Zwisler 2016-03-24 12:51 ` Jan Kara 2016-03-24 12:51 ` Jan Kara 2016-03-29 15:17 ` Ross Zwisler 2016-03-29 15:17 ` Ross Zwisler 2016-03-21 13:22 ` [PATCH 05/10] dax: Allow DAX code to replace exceptional entries Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-23 17:52 ` Ross Zwisler 2016-03-23 17:52 ` Ross Zwisler 2016-03-24 10:42 ` Jan Kara 2016-03-24 10:42 ` Jan Kara 2016-03-21 13:22 ` [PATCH 06/10] dax: Remove redundant inode size checks Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-23 21:08 ` Ross Zwisler 2016-03-23 21:08 ` Ross Zwisler 2016-03-21 13:22 ` [PATCH 07/10] dax: Disable huge page handling Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-23 20:50 ` Ross Zwisler 2016-03-23 20:50 ` Ross Zwisler 2016-03-24 12:56 ` Jan Kara 2016-03-24 12:56 ` Jan Kara 2016-03-21 13:22 ` [PATCH 08/10] dax: New fault locking Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-29 21:57 ` Ross Zwisler 2016-03-29 21:57 ` Ross Zwisler 2016-03-31 16:27 ` Jan Kara 2016-03-31 16:27 ` Jan Kara 2016-03-21 13:22 ` [PATCH 09/10] dax: Use radix tree entry lock to protect cow faults Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-21 19:11 ` Matthew Wilcox 2016-03-21 19:11 ` Matthew Wilcox 2016-03-22 7:03 ` Jan Kara 2016-03-22 7:03 ` Jan Kara 2016-03-29 22:18 ` Ross Zwisler 2016-03-29 22:18 ` Ross Zwisler 2016-03-21 13:22 ` [PATCH 10/10] dax: Remove i_mmap_lock protection Jan Kara 2016-03-21 13:22 ` Jan Kara 2016-03-29 22:17 ` Ross Zwisler 2016-03-29 22:17 ` Ross Zwisler 2016-03-21 17:41 ` [RFC v2] [PATCH 0/10] DAX page fault locking Matthew Wilcox 2016-03-21 17:41 ` Matthew Wilcox 2016-03-23 15:09 ` Jan Kara 2016-03-23 15:09 ` Jan Kara 2016-03-23 20:50 ` Matthew Wilcox 2016-03-23 20:50 ` Matthew Wilcox 2016-03-24 10:00 ` Matthew Wilcox 2016-03-24 10:00 ` Matthew Wilcox 2016-03-22 19:32 ` Ross Zwisler 2016-03-22 19:32 ` Ross Zwisler 2016-03-22 21:07 ` Toshi Kani 2016-03-22 21:07 ` Toshi Kani 2016-03-22 21:15 ` Dave Chinner 2016-03-22 21:15 ` Dave Chinner 2016-03-23 9:45 ` Jan Kara 2016-03-23 9:45 ` Jan Kara 2016-03-23 15:11 ` Toshi Kani 2016-03-23 15:11 ` Toshi Kani
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160323173945.GC5544@linux.intel.com \ --to=ross.zwisler@linux.intel.com \ --cc=jack@suse.cz \ --cc=linux-nvdimm@lists.01.org \ --cc=neilb@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.