* [PATCH] ext2, ext4: Fix issue with missing journal entry @ 2016-01-27 19:01 Ross Zwisler 2016-01-28 13:16 ` Jan Kara 0 siblings, 1 reply; 6+ messages in thread From: Ross Zwisler @ 2016-01-27 19:01 UTC (permalink / raw) To: linux-kernel Cc: Ross Zwisler, Theodore Ts'o, Dan Williams, Jan Kara, Matthew Wilcox, linux-ext4, linux-nvdimm As it is currently written ext4_dax_mkwrite() assumes that the call into __dax_mkwrite() will not have to do a block allocation so it doesn't create a journal entry. For a read that creates a zero page to cover a hole followed by a write that actually allocates storage this is incorrect. The ext4_dax_mkwrite() -> __dax_mkwrite() -> __dax_fault() path calls get_blocks() to allocate storage. Fix this by having the ->page_mkwrite fault handler call ext4_dax_fault() as this function already has all the logic needed to allocate a journal entry and call __dax_fault(). Also update the ext2 fault handlers in this same way to remove duplicate code and keep the logic between ext2 and ext4 the same. Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> --- fs/ext2/file.c | 19 +------------------ fs/ext4/file.c | 19 ++----------------- 2 files changed, 3 insertions(+), 35 deletions(-) diff --git a/fs/ext2/file.c b/fs/ext2/file.c index 2c88d68..c1400b1 100644 --- a/fs/ext2/file.c +++ b/fs/ext2/file.c @@ -80,23 +80,6 @@ static int ext2_dax_pmd_fault(struct vm_area_struct *vma, unsigned long addr, return ret; } -static int ext2_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) -{ - struct inode *inode = file_inode(vma->vm_file); - struct ext2_inode_info *ei = EXT2_I(inode); - int ret; - - sb_start_pagefault(inode->i_sb); - file_update_time(vma->vm_file); - down_read(&ei->dax_sem); - - ret = __dax_mkwrite(vma, vmf, ext2_get_block, NULL); - - up_read(&ei->dax_sem); - sb_end_pagefault(inode->i_sb); - return ret; -} - static int ext2_dax_pfn_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) { @@ -124,7 +107,7 @@ static int ext2_dax_pfn_mkwrite(struct vm_area_struct *vma, static const struct vm_operations_struct ext2_dax_vm_ops = { .fault = ext2_dax_fault, .pmd_fault = ext2_dax_pmd_fault, - .page_mkwrite = ext2_dax_mkwrite, + .page_mkwrite = ext2_dax_fault, .pfn_mkwrite = ext2_dax_pfn_mkwrite, }; diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 1126436..d2e8500 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -262,23 +262,8 @@ static int ext4_dax_pmd_fault(struct vm_area_struct *vma, unsigned long addr, return result; } -static int ext4_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) -{ - int err; - struct inode *inode = file_inode(vma->vm_file); - - sb_start_pagefault(inode->i_sb); - file_update_time(vma->vm_file); - down_read(&EXT4_I(inode)->i_mmap_sem); - err = __dax_mkwrite(vma, vmf, ext4_dax_mmap_get_block, NULL); - up_read(&EXT4_I(inode)->i_mmap_sem); - sb_end_pagefault(inode->i_sb); - - return err; -} ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ext2, ext4: Fix issue with missing journal entry 2016-01-27 19:01 [PATCH] ext2, ext4: Fix issue with missing journal entry Ross Zwisler @ 2016-01-28 13:16 ` Jan Kara 2016-01-28 16:32 ` Ross Zwisler 0 siblings, 1 reply; 6+ messages in thread From: Jan Kara @ 2016-01-28 13:16 UTC (permalink / raw) To: Ross Zwisler Cc: linux-kernel, Theodore Ts'o, Dan Williams, Jan Kara, Matthew Wilcox, linux-ext4, linux-nvdimm On Wed 27-01-16 12:01:48, Ross Zwisler wrote: > As it is currently written ext4_dax_mkwrite() assumes that the call into > __dax_mkwrite() will not have to do a block allocation so it doesn't create > a journal entry. For a read that creates a zero page to cover a hole > followed by a write that actually allocates storage this is incorrect. The > ext4_dax_mkwrite() -> __dax_mkwrite() -> __dax_fault() path calls > get_blocks() to allocate storage. > > Fix this by having the ->page_mkwrite fault handler call ext4_dax_fault() > as this function already has all the logic needed to allocate a journal > entry and call __dax_fault(). > > Also update the ext2 fault handlers in this same way to remove duplicate > code and keep the logic between ext2 and ext4 the same. > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> Ah, ok, you are right. The patch looks good but Matthew is reworking the area more (so ext4_da_mkwrite() is likely to return) so it this worth it? Or do you expect Matthew's patches to land much later? Honza > --- > fs/ext2/file.c | 19 +------------------ > fs/ext4/file.c | 19 ++----------------- > 2 files changed, 3 insertions(+), 35 deletions(-) > > diff --git a/fs/ext2/file.c b/fs/ext2/file.c > index 2c88d68..c1400b1 100644 > --- a/fs/ext2/file.c > +++ b/fs/ext2/file.c > @@ -80,23 +80,6 @@ static int ext2_dax_pmd_fault(struct vm_area_struct *vma, unsigned long addr, > return ret; > } > > -static int ext2_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) > -{ > - struct inode *inode = file_inode(vma->vm_file); > - struct ext2_inode_info *ei = EXT2_I(inode); > - int ret; > - > - sb_start_pagefault(inode->i_sb); > - file_update_time(vma->vm_file); > - down_read(&ei->dax_sem); > - > - ret = __dax_mkwrite(vma, vmf, ext2_get_block, NULL); > - > - up_read(&ei->dax_sem); > - sb_end_pagefault(inode->i_sb); > - return ret; > -} > - > static int ext2_dax_pfn_mkwrite(struct vm_area_struct *vma, > struct vm_fault *vmf) > { > @@ -124,7 +107,7 @@ static int ext2_dax_pfn_mkwrite(struct vm_area_struct *vma, > static const struct vm_operations_struct ext2_dax_vm_ops = { > .fault = ext2_dax_fault, > .pmd_fault = ext2_dax_pmd_fault, > - .page_mkwrite = ext2_dax_mkwrite, > + .page_mkwrite = ext2_dax_fault, > .pfn_mkwrite = ext2_dax_pfn_mkwrite, > }; > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > index 1126436..d2e8500 100644 > --- a/fs/ext4/file.c > +++ b/fs/ext4/file.c > @@ -262,23 +262,8 @@ static int ext4_dax_pmd_fault(struct vm_area_struct *vma, unsigned long addr, > return result; > } > > -static int ext4_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) > -{ > - int err; > - struct inode *inode = file_inode(vma->vm_file); > - > - sb_start_pagefault(inode->i_sb); > - file_update_time(vma->vm_file); > - down_read(&EXT4_I(inode)->i_mmap_sem); > - err = __dax_mkwrite(vma, vmf, ext4_dax_mmap_get_block, NULL); > - up_read(&EXT4_I(inode)->i_mmap_sem); > - sb_end_pagefault(inode->i_sb); > - > - return err; > -} > - > /* > - * Handle write fault for VM_MIXEDMAP mappings. Similarly to ext4_dax_mkwrite() > + * Handle write fault for VM_MIXEDMAP mappings. Similarly to ext4_dax_fault() > * handler we check for races agaist truncate. Note that since we cycle through > * i_mmap_sem, we are sure that also any hole punching that began before we > * were called is finished by now and so if it included part of the file we > @@ -311,7 +296,7 @@ static int ext4_dax_pfn_mkwrite(struct vm_area_struct *vma, > static const struct vm_operations_struct ext4_dax_vm_ops = { > .fault = ext4_dax_fault, > .pmd_fault = ext4_dax_pmd_fault, > - .page_mkwrite = ext4_dax_mkwrite, > + .page_mkwrite = ext4_dax_fault, > .pfn_mkwrite = ext4_dax_pfn_mkwrite, > }; > #else > -- > 2.5.0 > > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext2, ext4: Fix issue with missing journal entry 2016-01-28 13:16 ` Jan Kara @ 2016-01-28 16:32 ` Ross Zwisler 2016-02-24 20:44 ` Ross Zwisler 0 siblings, 1 reply; 6+ messages in thread From: Ross Zwisler @ 2016-01-28 16:32 UTC (permalink / raw) To: Jan Kara Cc: Ross Zwisler, linux-kernel, Theodore Ts'o, Dan Williams, Jan Kara, Matthew Wilcox, linux-ext4, linux-nvdimm On Thu, Jan 28, 2016 at 02:16:30PM +0100, Jan Kara wrote: > On Wed 27-01-16 12:01:48, Ross Zwisler wrote: > > As it is currently written ext4_dax_mkwrite() assumes that the call into > > __dax_mkwrite() will not have to do a block allocation so it doesn't create > > a journal entry. For a read that creates a zero page to cover a hole > > followed by a write that actually allocates storage this is incorrect. The > > ext4_dax_mkwrite() -> __dax_mkwrite() -> __dax_fault() path calls > > get_blocks() to allocate storage. > > > > Fix this by having the ->page_mkwrite fault handler call ext4_dax_fault() > > as this function already has all the logic needed to allocate a journal > > entry and call __dax_fault(). > > > > Also update the ext2 fault handlers in this same way to remove duplicate > > code and keep the logic between ext2 and ext4 the same. > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > Ah, ok, you are right. The patch looks good but Matthew is reworking the > area more (so ext4_da_mkwrite() is likely to return) so it this worth it? > Or do you expect Matthew's patches to land much later? Yep, Matthew is in the process of reworking all of the DAX fault handling. I was thinking that we might want to take this patch for v4.5, since it fixes a bug that I'm guessing could lead to some sort of corruption (lack of a journal entry entry for an allocating write), and then Matthew's reworks would land in v4.6? > > --- > > fs/ext2/file.c | 19 +------------------ > > fs/ext4/file.c | 19 ++----------------- > > 2 files changed, 3 insertions(+), 35 deletions(-) > > > > diff --git a/fs/ext2/file.c b/fs/ext2/file.c > > index 2c88d68..c1400b1 100644 > > --- a/fs/ext2/file.c > > +++ b/fs/ext2/file.c > > @@ -80,23 +80,6 @@ static int ext2_dax_pmd_fault(struct vm_area_struct *vma, unsigned long addr, > > return ret; > > } > > > > -static int ext2_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) > > -{ > > - struct inode *inode = file_inode(vma->vm_file); > > - struct ext2_inode_info *ei = EXT2_I(inode); > > - int ret; > > - > > - sb_start_pagefault(inode->i_sb); > > - file_update_time(vma->vm_file); > > - down_read(&ei->dax_sem); > > - > > - ret = __dax_mkwrite(vma, vmf, ext2_get_block, NULL); > > - > > - up_read(&ei->dax_sem); > > - sb_end_pagefault(inode->i_sb); > > - return ret; > > -} > > - > > static int ext2_dax_pfn_mkwrite(struct vm_area_struct *vma, > > struct vm_fault *vmf) > > { > > @@ -124,7 +107,7 @@ static int ext2_dax_pfn_mkwrite(struct vm_area_struct *vma, > > static const struct vm_operations_struct ext2_dax_vm_ops = { > > .fault = ext2_dax_fault, > > .pmd_fault = ext2_dax_pmd_fault, > > - .page_mkwrite = ext2_dax_mkwrite, > > + .page_mkwrite = ext2_dax_fault, > > .pfn_mkwrite = ext2_dax_pfn_mkwrite, > > }; > > > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > > index 1126436..d2e8500 100644 > > --- a/fs/ext4/file.c > > +++ b/fs/ext4/file.c > > @@ -262,23 +262,8 @@ static int ext4_dax_pmd_fault(struct vm_area_struct *vma, unsigned long addr, > > return result; > > } > > > > -static int ext4_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) > > -{ > > - int err; > > - struct inode *inode = file_inode(vma->vm_file); > > - > > - sb_start_pagefault(inode->i_sb); > > - file_update_time(vma->vm_file); > > - down_read(&EXT4_I(inode)->i_mmap_sem); > > - err = __dax_mkwrite(vma, vmf, ext4_dax_mmap_get_block, NULL); > > - up_read(&EXT4_I(inode)->i_mmap_sem); > > - sb_end_pagefault(inode->i_sb); > > - > > - return err; > > -} > > - > > /* > > - * Handle write fault for VM_MIXEDMAP mappings. Similarly to ext4_dax_mkwrite() > > + * Handle write fault for VM_MIXEDMAP mappings. Similarly to ext4_dax_fault() > > * handler we check for races agaist truncate. Note that since we cycle through > > * i_mmap_sem, we are sure that also any hole punching that began before we > > * were called is finished by now and so if it included part of the file we > > @@ -311,7 +296,7 @@ static int ext4_dax_pfn_mkwrite(struct vm_area_struct *vma, > > static const struct vm_operations_struct ext4_dax_vm_ops = { > > .fault = ext4_dax_fault, > > .pmd_fault = ext4_dax_pmd_fault, > > - .page_mkwrite = ext4_dax_mkwrite, > > + .page_mkwrite = ext4_dax_fault, > > .pfn_mkwrite = ext4_dax_pfn_mkwrite, > > }; > > #else > > -- > > 2.5.0 > > > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext2, ext4: Fix issue with missing journal entry 2016-01-28 16:32 ` Ross Zwisler @ 2016-02-24 20:44 ` Ross Zwisler 2016-02-25 8:37 ` Jan Kara 0 siblings, 1 reply; 6+ messages in thread From: Ross Zwisler @ 2016-02-24 20:44 UTC (permalink / raw) To: Ross Zwisler Cc: Jan Kara, linux-kernel, Theodore Ts'o, Dan Williams, Jan Kara, Matthew Wilcox, linux-ext4, linux-nvdimm On Thu, Jan 28, 2016 at 09:32:11AM -0700, Ross Zwisler wrote: > On Thu, Jan 28, 2016 at 02:16:30PM +0100, Jan Kara wrote: > > On Wed 27-01-16 12:01:48, Ross Zwisler wrote: > > > As it is currently written ext4_dax_mkwrite() assumes that the call into > > > __dax_mkwrite() will not have to do a block allocation so it doesn't create > > > a journal entry. For a read that creates a zero page to cover a hole > > > followed by a write that actually allocates storage this is incorrect. The > > > ext4_dax_mkwrite() -> __dax_mkwrite() -> __dax_fault() path calls > > > get_blocks() to allocate storage. > > > > > > Fix this by having the ->page_mkwrite fault handler call ext4_dax_fault() > > > as this function already has all the logic needed to allocate a journal > > > entry and call __dax_fault(). > > > > > > Also update the ext2 fault handlers in this same way to remove duplicate > > > code and keep the logic between ext2 and ext4 the same. > > > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > > > Ah, ok, you are right. The patch looks good but Matthew is reworking the > > area more (so ext4_da_mkwrite() is likely to return) so it this worth it? > > Or do you expect Matthew's patches to land much later? > > Yep, Matthew is in the process of reworking all of the DAX fault handling. > > I was thinking that we might want to take this patch for v4.5, since it fixes > a bug that I'm guessing could lead to some sort of corruption (lack of a > journal entry entry for an allocating write), and then Matthew's reworks would > land in v4.6? Hey Jan, Looks like this patch didn't ever get merged for v4.5? Is it still queued for v4.6? Thanks, - Ross ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext2, ext4: Fix issue with missing journal entry 2016-02-24 20:44 ` Ross Zwisler @ 2016-02-25 8:37 ` Jan Kara 2016-02-27 19:21 ` Theodore Ts'o 0 siblings, 1 reply; 6+ messages in thread From: Jan Kara @ 2016-02-25 8:37 UTC (permalink / raw) To: Ross Zwisler Cc: Jan Kara, linux-kernel, Theodore Ts'o, Dan Williams, Jan Kara, Matthew Wilcox, linux-ext4, linux-nvdimm On Wed 24-02-16 13:44:46, Ross Zwisler wrote: > On Thu, Jan 28, 2016 at 09:32:11AM -0700, Ross Zwisler wrote: > > On Thu, Jan 28, 2016 at 02:16:30PM +0100, Jan Kara wrote: > > > On Wed 27-01-16 12:01:48, Ross Zwisler wrote: > > > > As it is currently written ext4_dax_mkwrite() assumes that the call into > > > > __dax_mkwrite() will not have to do a block allocation so it doesn't create > > > > a journal entry. For a read that creates a zero page to cover a hole > > > > followed by a write that actually allocates storage this is incorrect. The > > > > ext4_dax_mkwrite() -> __dax_mkwrite() -> __dax_fault() path calls > > > > get_blocks() to allocate storage. > > > > > > > > Fix this by having the ->page_mkwrite fault handler call ext4_dax_fault() > > > > as this function already has all the logic needed to allocate a journal > > > > entry and call __dax_fault(). > > > > > > > > Also update the ext2 fault handlers in this same way to remove duplicate > > > > code and keep the logic between ext2 and ext4 the same. > > > > > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > > > > > Ah, ok, you are right. The patch looks good but Matthew is reworking the > > > area more (so ext4_da_mkwrite() is likely to return) so it this worth it? > > > Or do you expect Matthew's patches to land much later? > > > > Yep, Matthew is in the process of reworking all of the DAX fault handling. > > > > I was thinking that we might want to take this patch for v4.5, since it fixes > > a bug that I'm guessing could lead to some sort of corruption (lack of a > > journal entry entry for an allocating write), and then Matthew's reworks would > > land in v4.6? > > Hey Jan, > > Looks like this patch didn't ever get merged for v4.5? Is it still queued for > v4.6? Ted has been pretty busy lately and we probably didn't make it sufficiently clear that he should pick up this patch. Ted, can you please pick up this patch and push it to 4.5? Thanks. Feel free to add my: Reviewed-by: Jan Kara <jack@suse.cz> Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext2, ext4: Fix issue with missing journal entry 2016-02-25 8:37 ` Jan Kara @ 2016-02-27 19:21 ` Theodore Ts'o 0 siblings, 0 replies; 6+ messages in thread From: Theodore Ts'o @ 2016-02-27 19:21 UTC (permalink / raw) To: Jan Kara Cc: Ross Zwisler, linux-kernel, Dan Williams, Jan Kara, Matthew Wilcox, linux-ext4, linux-nvdimm On Thu, Feb 25, 2016 at 09:37:28AM +0100, Jan Kara wrote: > > Looks like this patch didn't ever get merged for v4.5? Is it still queued for > > v4.6? > > Ted has been pretty busy lately and we probably didn't make it sufficiently > clear that he should pick up this patch. Ted, can you please pick up this > patch and push it to 4.5? Thanks. Feel free to add my: > > Reviewed-by: Jan Kara <jack@suse.cz> Sorry, yes, I missed this one since most of the DAX patches have been going through other trees. I've just sent a pull request to Linus. - Ted ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-27 19:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-27 19:01 [PATCH] ext2, ext4: Fix issue with missing journal entry Ross Zwisler 2016-01-28 13:16 ` Jan Kara 2016-01-28 16:32 ` Ross Zwisler 2016-02-24 20:44 ` Ross Zwisler 2016-02-25 8:37 ` Jan Kara 2016-02-27 19:21 ` Theodore Ts'o
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).