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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux