lkml.org 
[lkml]   [2014]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v7 07/22] Replace the XIP page fault handler with the DAX page fault handler
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@suse.cz>
SUSE Labs, CR


\
 
 \ /
  Last update: 2014-04-14 18:21    [W:0.086 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site