by Matthew Wilcox

[permalink] [raw]

Subject: Re: [PATCH v6 07/22] Replace the XIP page fault handler with the DAX page fault handler

On Fri, Feb 28, 2014 at 10:49:31AM -0700, Toshi Kani wrote:
> On Tue, 2014-02-25 at 09:18 -0500, Matthew Wilcox wrote:
> > Instead of calling aops->get_xip_mem from the fault handler, the
> > filesystem passes a get_block_t that is used to find the appropriate
> > blocks.
> :
> > +static int do_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
> > + get_block_t get_block)
> > +{
> > + struct file *file = vma->vm_file;
> > + struct inode *inode = file_inode(file);
> > + struct address_space *mapping = file->f_mapping;
> > + struct buffer_head bh;
> > + unsigned long vaddr = (unsigned long)vmf->virtual_address;
> > + sector_t block;
> > + pgoff_t size;
> > + unsigned long pfn;
> > + int error;
> > +
> > + size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
> > + if (vmf->pgoff >= size)
> > + return VM_FAULT_SIGBUS;
> > +
> > + memset(&bh, 0, sizeof(bh));
> > + block = (sector_t)vmf->pgoff << (PAGE_SHIFT - inode->i_blkbits);
> > + bh.b_size = PAGE_SIZE;
> > + error = get_block(inode, block, &bh, 0);
> > + if (error || bh.b_size < PAGE_SIZE)
> > + return VM_FAULT_SIGBUS;
>
> I am learning the code and have some questions.

Hi Toshi,

Glad to see you're looking at it. Let me try to help ...

> The original code,
> xip_file_fault(), jumps to found: and calls vm_insert_mixed() when
> get_xip_mem(,,0,,) succeeded. If get_xip_mem() returns -ENODATA, it
> calls either get_xip_mem(,,1,,) or xip_sparse_page(). In this new
> function, it looks to me that get_block(,,,0) returns 0 for both cases
> (success and -ENODATA previously), which are dealt in the same way. Is
> that right? If so, is there any reason for the change?

Yes, get_xip_mem() returned -ENODATA for a hole. That was a suboptimal
interface because filesystems are actually capable of returning more
information than that, eg how long the hole is (ext4 *doesn't*, but I
consider that to be a bug).

I don't get to decide what the get_block() interface looks like. It's the
standard way that the VFS calls back into the filesystem and has been
around for probably close to twenty years at this point. I'm still trying
to understand exactly what the contract is for get_blocks() ... I have
a document that I'm working on to try to explain it, but it's tough going!

> Also, isn't it
> possible to call get_block(,,,1) even if get_block(,,,0) found a block?

The code in question looks like this:

error = get_block(inode, block, &bh, 0);
if (error || bh.b_size < PAGE_SIZE)
goto sigbus;

if (!buffer_written(&bh) && !vmf->cow_page) {
if (vmf->flags & FAULT_FLAG_WRITE) {
error = get_block(inode, block, &bh, 1);

where buffer_written is defined as:
return buffer_mapped(bh) && !buffer_unwritten(bh);

Doing some boolean algebra, that's:

if (!buffer_mapped || buffer_unwritten)

In either case, we want to tell the filesystem that we're writing to
this block. At least, that's my current understanding of the get_block()
interface. I'm open to correction here!

2014-02-28 22:25:07

by Toshi Kani

[permalink] [raw]

Subject: Re: [PATCH v6 07/22] Replace the XIP page fault handler with the DAX page fault handler

On Fri, 2014-02-28 at 15:20 -0500, Matthew Wilcox wrote:
> On Fri, Feb 28, 2014 at 10:49:31AM -0700, Toshi Kani wrote:
> > On Tue, 2014-02-25 at 09:18 -0500, Matthew Wilcox wrote:
:
> Glad to see you're looking at it. Let me try to help ...

Hi Matt,

Thanks for the help. This is really a nice work, and I am hoping to
help it... (in some day! :-)

> > The original code,
> > xip_file_fault(), jumps to found: and calls vm_insert_mixed() when
> > get_xip_mem(,,0,,) succeeded. If get_xip_mem() returns -ENODATA, it
> > calls either get_xip_mem(,,1,,) or xip_sparse_page(). In this new
> > function, it looks to me that get_block(,,,0) returns 0 for both cases
> > (success and -ENODATA previously), which are dealt in the same way. Is
> > that right? If so, is there any reason for the change?
>
> Yes, get_xip_mem() returned -ENODATA for a hole. That was a suboptimal
> interface because filesystems are actually capable of returning more
> information than that, eg how long the hole is (ext4 *doesn't*, but I
> consider that to be a bug).
>
> I don't get to decide what the get_block() interface looks like. It's the
> standard way that the VFS calls back into the filesystem and has been
> around for probably close to twenty years at this point. I'm still trying
> to understand exactly what the contract is for get_blocks() ... I have
> a document that I'm working on to try to explain it, but it's tough going!

Got it. Yes, get_block() is a beast for file system newbie like me.
Thanks for working on the document.

> > Also, isn't it
> > possible to call get_block(,,,1) even if get_block(,,,0) found a block?
>
> The code in question looks like this:
>
> error = get_block(inode, block, &bh, 0);
> if (error || bh.b_size < PAGE_SIZE)
> goto sigbus;
>
> if (!buffer_written(&bh) && !vmf->cow_page) {
> if (vmf->flags & FAULT_FLAG_WRITE) {
> error = get_block(inode, block, &bh, 1);
>
> where buffer_written is defined as:
> return buffer_mapped(bh) && !buffer_unwritten(bh);
>
> Doing some boolean algebra, that's:
>
> if (!buffer_mapped || buffer_unwritten)

Oh, I see! When the first get_block(,,,0) succeeded, this buffer is
mapped. So, it won't go into this path.

> In either case, we want to tell the filesystem that we're writing to
> this block. At least, that's my current understanding of the get_block()
> interface. I'm open to correction here!

Thanks again!
-Toshi