On Tue, Oct 27, 2020 at 06:15:52PM +0000, Christoph Hellwig wrote: > On Tue, Oct 20, 2020 at 12:21:50PM -0400, Brian Foster wrote: > > Ugh, so the above doesn't quite describe historical behavior. > > block_truncate_page() converts an unwritten block if a page exists > > (dirty or not), but bails out if a page doesn't exist. We could still do > > the above, but if we wanted something more intelligent I think we need > > to check for a page before we get the mapping to know whether we can > > safely skip an unwritten block or need to write over it. Otherwise if we > > check for a page within the actor, we have no way of knowing whether > > there was a (possibly dirty) page that had been written back and/or > > reclaimed since ->iomap_begin(). If we check for the page first, I think > > that the iolock/mmaplock in the truncate path ensures that a page can't > > be added before we complete. We might be able to take that further and > > check for a dirty || writeback page, but that might be safer as a > > separate patch. See the (compile tested only) diff below for an idea of > > what I was thinking. > > The idea looks reasonable, but a few comment below: > JFYI, I had posted an implementation of this idea here[1] and followed up with some details on a similar COW related issue that was exposed once the unwritten variant was addressed. I was reasoning about a slightly different approach that might more clearly facilitate handling both scenarios, but I think I mentioned to Darrick offline that this all has me back to preferring the original patch to flush the new EOF block first, at least as a first step. I have a couple other fixes (one being the discard_page() patch you've already commented on) related to iomap and I'm going to be offline for a few weeks after this week so I'll try to collect them in a series and get them posted together soon.. Brian [1] https://lore.kernel.org/linux-fsdevel/20201021133329.1337689-1-bfoster@xxxxxxxxxx/ > > +struct iomap_trunc_priv { > > + bool *did_zero; > > I don't think there is any point on using a pointer here, when we > can trivially copy out the scalar value. > > > + bool has_page; > > The naming of this flag really confuses me. Maybe has_data or > in_pagecache might be better options? > > > +static loff_t > > +iomap_truncate_page_actor(struct inode *inode, loff_t pos, loff_t count, > > + void *data, struct iomap *iomap, struct iomap *srcmap) > > +{ > > + struct iomap_trunc_priv *priv = data; > > + unsigned offset; > > + int status; > > + > > + if (srcmap->type == IOMAP_HOLE) > > + return count; > > + if (srcmap->type == IOMAP_UNWRITTEN && !priv->has_page) > > + return count; > > Maybe add a comment here to explain why priv->has_page matters? > > > + > > + offset = offset_in_page(pos); > > I'd move this on the initialization line. > > > + ret = iomap_apply(inode, pos, blocksize - off, IOMAP_ZERO, ops, &priv, > > + iomap_truncate_page_actor); > > + if (ret <= 0) > > + return ret; > > The check could just be < 0 and would be a little more obvious. >
