On Wed, Oct 09, 2019 at 08:28:24AM +0200, Christoph Hellwig wrote:
> On Tue, Oct 08, 2019 at 08:00:44AM -0700, Darrick J. Wong wrote:
> > > unsigned long vaddr = vmf->address;
> > > loff_t pos = (loff_t)vmf->pgoff << PAGE_SHIFT;
> > > struct iomap iomap = { 0 };
> >
> > Does this definition ^^^^^ need to be converted too? You convert the
> > one in iomap_apply()...
>
> Doesn't strictly need to, but it sure would look nicer and fit the theme.
>
> > /*
> > * The @iomap and @srcmap parameters should be set to a hole
> > * prior to calling ->iomap_begin.
> > */
> > #define IOMAP_EMPTY_RECORD { .type = IOMAP_HOLE }
> >
> > ...and later...
> >
> > struct iomap srcmap = IOMAP_EMPTY_RECORD;
> >
> > ..but meh, I'm not sure that adds much.
>
> I don't really see the point.
Yeah. Agreed.
> > > unsigned flags = IOMAP_FAULT;
> > > int error, major = 0;
> > > bool write = vmf->flags & FAULT_FLAG_WRITE;
> > > @@ -1292,7 +1293,7 @@ static vm_fault_t dax_iomap_pte_fault(struct vm_fault *vmf, pfn_t *pfnp,
> > > * the file system block size to be equal the page size, which means
> > > * that we never have to deal with more than a single extent here.
> > > */
> > > - error = ops->iomap_begin(inode, pos, PAGE_SIZE, flags, &iomap);
> > > + error = ops->iomap_begin(inode, pos, PAGE_SIZE, flags, &iomap, &srcmap);
> >
> > ->iomap_begin callers are never supposed to touch srcmap, right?
> > Maybe we ought to check that srcmap.io_type == HOLE, at least until
> > someone fixes this code to dax-copy the data from srcmap to iomap?
>
> What do you mean with touch? ->iomap_begin fills it out and then the
> caller looks at it, at least for places that can deal with
> read-modify-write operations (DAX currently can't).
Yes, I grok that the DAX code should never get fed a shared mapping, but
maybe we ought to have a WARN_ON_ONCE just in case some filesystem AI
programmer decides to backport a fs patch that results in sending a
non-hole srcmap back to the dax iomap callers. /We/ know that you
should never do this, but does the AI know? <grumble>
(Yeah, pure paranoia on my part :P)
--D
