| Message ID | 54F820E2.9060109@plexistor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/05/2015 11:24 AM, Boaz Harrosh wrote: > > [v1] > Without this patch, c/mtime is not updated correctly when mmap'ed page is > first read from and then written to. > > A new xfstest is submitted for testing this (generic/080) > > [v2] > Jan Kara has pointed out that if we add the > sb_start/end_pagefault pair in the new pfn_mkwrite we > are then fixing another bug where: A user could start > writing to the page while filesystem is frozen. > Thanks Jan. Just as curiosity, does the freezing code goes and turns all mappings into read-only, Also for pfn mapping? Do you think there is already an xfstest freezing test that should now fail, and will succeed after this patch (v2). Something like: * mmap-read/write before the freeze * freeze the fs * Another thread tries to mmap-write, should get stuck * unfreeze the fs * Now mmap-writer continues Thanks again Boaz > CC: Jan Kara <jack@suse.cz> > Signed-off-by: Yigal Korman <yigal@plexistor.com> > Signed-off-by: Boaz Harrosh <boaz@plexistor.com> <>
On Thu 05-03-15 11:32:25, Boaz Harrosh wrote: > On 03/05/2015 11:24 AM, Boaz Harrosh wrote: > > > > [v1] > > Without this patch, c/mtime is not updated correctly when mmap'ed page is > > first read from and then written to. > > > > A new xfstest is submitted for testing this (generic/080) > > > > [v2] > > Jan Kara has pointed out that if we add the > > sb_start/end_pagefault pair in the new pfn_mkwrite we > > are then fixing another bug where: A user could start > > writing to the page while filesystem is frozen. > > > > Thanks Jan. > > Just as curiosity, does the freezing code goes and turns all mappings > into read-only, Also for pfn mapping? Hum, that's a good question. Probably we don't end up doing that. For normal filesystems we sync all inodes which also writeprotects all pages (in clear_page_dirty_for_io() - for normal filesystems we know that if page is writeably mapped it is dirty). However this won't happen for pfn mapping as we don't have dirty pages. So we probably need dax_freeze() implementation that will walk through all inodes with writeable mappings and writeprotect them. > Do you think there is already an xfstest freezing test that should now > fail, and will succeed after this patch (v2). Something like: > * mmap-read/write before the freeze > * freeze the fs > * Another thread tries to mmap-write, should get stuck > * unfreeze the fs > * Now mmap-writer continues I don't remember there would be any test to specifically test this. Honza
On 03/05/2015 12:35 PM, Jan Kara wrote: > On Thu 05-03-15 11:32:25, Boaz Harrosh wrote: >> On 03/05/2015 11:24 AM, Boaz Harrosh wrote: <> >> >> Just as curiosity, does the freezing code goes and turns all mappings >> into read-only, Also for pfn mapping? > Hum, that's a good question. Probably we don't end up doing that. For > > normal filesystems we sync all inodes which also writeprotects all pages > (in clear_page_dirty_for_io() - for normal filesystems we know that if page > is writeably mapped it is dirty). However this won't happen for pfn > mapping as we don't have dirty pages. So we probably need dax_freeze() > implementation that will walk through all inodes with writeable mappings and > writeprotect them. > I'll go head and try my shot on implementing a dax_freeze(). But I will please need help with where to call it from. Probably something like: if (IS_DAX(inode)) dax_freeze(inode); else sync(inode) So to share the for-all-inodes-in-sb loop. And also the IS_DAX(inode) is per inode, for example dirs need the regular sync (if they are using page cache) >> Do you think there is already an xfstest freezing test that should now >> fail, and will succeed after this patch (v2). Something like: >> * mmap-read/write before the freeze >> * freeze the fs >> * Another thread tries to mmap-write, should get stuck >> * unfreeze the fs >> * Now mmap-writer continues > I don't remember there would be any test to specifically test this. > OK Thanks, I was hopping we should already test for mmap vs freeze. This is not special for our case. Actually mmap is the most fragile access. > Honza > Thanks Boaz
On Thu 05-03-15 12:47:30, Boaz Harrosh wrote: > On 03/05/2015 12:35 PM, Jan Kara wrote: > > On Thu 05-03-15 11:32:25, Boaz Harrosh wrote: > >> On 03/05/2015 11:24 AM, Boaz Harrosh wrote: > <> > >> > >> Just as curiosity, does the freezing code goes and turns all mappings > >> into read-only, Also for pfn mapping? > > Hum, that's a good question. Probably we don't end up doing that. For > > > > normal filesystems we sync all inodes which also writeprotects all pages > > (in clear_page_dirty_for_io() - for normal filesystems we know that if page > > is writeably mapped it is dirty). However this won't happen for pfn > > mapping as we don't have dirty pages. So we probably need dax_freeze() > > implementation that will walk through all inodes with writeable mappings and > > writeprotect them. > > > > I'll go head and try my shot on implementing a dax_freeze(). But I will > please need help with where to call it from. > > Probably something like: > if (IS_DAX(inode)) > dax_freeze(inode); > else > sync(inode) We normally call sync_filesystem() from fs/superblock:freeze_super(). For DAX filesystems we'd also need to call the special function after that. Maybe dax_freeze() isn't the best name. It could be called something like dax_writeprotect(sb) or something like that. Honza
diff --git a/fs/dax.c b/fs/dax.c index ed1619e..d0bd1f4 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -464,6 +464,23 @@ int dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf, EXPORT_SYMBOL_GPL(dax_fault); /** + * dax_pfn_mkwrite - handle first write to DAX page + * @vma: The virtual memory area where the fault occurred + * @vmf: The description of the fault + * + */ +int dax_pfn_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) +{ + struct super_block *sb = file_inode(vma->vm_file)->i_sb; + + sb_start_pagefault(sb); + file_update_time(vma->vm_file); + sb_end_pagefault(sb); + return VM_FAULT_NOPAGE; +} +EXPORT_SYMBOL_GPL(dax_pfn_mkwrite); + +/** * dax_zero_page_range - zero a range within a page of a DAX file * @inode: The file being truncated * @from: The file offset that is being truncated to diff --git a/fs/ext2/file.c b/fs/ext2/file.c index e317017..866a3ce 100644 --- a/fs/ext2/file.c +++ b/fs/ext2/file.c @@ -39,6 +39,7 @@ static int ext2_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) static const struct vm_operations_struct ext2_dax_vm_ops = { .fault = ext2_dax_fault, .page_mkwrite = ext2_dax_mkwrite, + .pfn_mkwrite = dax_pfn_mkwrite, }; static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma) diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 33a09da..b43a7a6 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -206,6 +206,7 @@ static int ext4_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) static const struct vm_operations_struct ext4_dax_vm_ops = { .fault = ext4_dax_fault, .page_mkwrite = ext4_dax_mkwrite, + .pfn_mkwrite = dax_pfn_mkwrite, }; #else #define ext4_dax_vm_ops ext4_file_vm_ops diff --git a/include/linux/fs.h b/include/linux/fs.h index b4d71b5..24af817 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2597,6 +2597,7 @@ int dax_clear_blocks(struct inode *, sector_t block, long size); int dax_zero_page_range(struct inode *, loff_t from, unsigned len, get_block_t); int dax_truncate_page(struct inode *, loff_t from, get_block_t); int dax_fault(struct vm_area_struct *, struct vm_fault *, get_block_t); +int dax_pfn_mkwrite(struct vm_area_struct *, struct vm_fault *); #define dax_mkwrite(vma, vmf, gb) dax_fault(vma, vmf, gb) #ifdef CONFIG_BLOCK