On Sun 13-04-14 07:21:32, Matthew Wilcox wrote:
> On Wed, Apr 09, 2014 at 11:12:03PM +0200, Jan Kara wrote:
> > This would be fine except that unmap_mapping_range() grabs i_mmap_mutex
> > again :-|. But it might be easier to provide a version of that function
> > which assumes i_mmap_mutex is already locked than what I was suggesting.
>
> *sigh*. I knew that once ... which was why the call was after dropping
> the lock. OK, another try at fixing the problem; handle it down in the
> insert_pfn code:
OK, that change looks OK to me (although you might want to introduce
vm_replace_mixed() in a separate patch).
> > > > > +int dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
> > > > > + get_block_t get_block)
> > > > > +{
> > > > > + int result;
> > > > > + struct super_block *sb = file_inode(vma->vm_file)->i_sb;
> > > > > +
> > > > > + sb_start_pagefault(sb);
> > > > You don't need any filesystem freeze protection for the fault handler
> > > > since that's not going to modify the filesystem.
> > >
> > > Err ... we might allocate a block as a result of doing a write to a hole.
> > > Or does that not count as 'modifying the filesystem' in this context?
> > Ah, it does. But it would be nice to avoid doing sb_start_pagefault() if
> > it's not a write fault - because you don't want to block reading from a
> > frozen filesystem (imagine what would happen when you freeze your root
> > filesystem to do a snapshot...).
> >
> > I have somewhat a mindset of standard pagecache mmap where filemap_fault()
> > only reads in data regardless of FAULT_FLAG_WRITE setting so I was confused
> > by your difference :).
>
> Understood! So this should work:
>
> diff --git a/fs/dax.c b/fs/dax.c
> index 2453025..e4d00fc 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -431,10 +431,13 @@ int dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
> int result;
> struct super_block *sb = file_inode(vma->vm_file)->i_sb;
>
> - sb_start_pagefault(sb);
> - file_update_time(vma->vm_file);
> + if (vmf->flags & FAULT_FLAG_WRITE) {
> + sb_start_pagefault(sb);
> + file_update_time(vma->vm_file);
> + }
Yup, this looks good to me. Later if we find file_update_time() is
slowing down faults too much, we can defer the actual update to msync()
/ close() time (POSIX actually allows that). But that's definitely for
future.
> result = do_dax_fault(vma, vmf, get_block);
> - sb_end_pagefault(sb);
> + if (vmf->flags & FAULT_FLAG_WRITE)
> + sb_end_pagefault(sb);
>
> return result;
> }
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
