Re: [PATCH 08/20] iomap: use a srcmap for a read-modify-write I/O

[Christoph Hellwig <hch@lst.de>]

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.

> >  	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).