All of lore.kernel.org
 help / color / mirror / Atom feed
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. :)

  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: link
Be 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.