On Thu, Jan 17, 2019 at 11:35:17AM -0500, Brian Foster wrote: > Hmm, it would be nice if these fixes were separate from the whole > always_cow thing. Some initial thoughts on a quick look through the > first few patches on the v3 post: We can always skip the last patch. It just helps to really nicely show a lot of the problems that are otherwise hard to reproduce, but already exist. FYI, I just resent it like a minute before reading your mail. > 1. It's probably best to drop your xfs_trim_extent_eof() changes as I > have a stable patch to add a couple more calls and then I subsequently > remove the whole thing going forward. Refactoring it is just churn at > this point. Sure. > 2. The whole explicit race with truncate detection looks rather involved > to me at first glance. I'm trying to avoid relying on i_size at all for > this because it doesn't seem like a reliable approach. E.g., Dave > described a hole punch vector for the same fundamental problem this > series is trying to address: > > https://marc.info/?l=linux-xfs&m=154692641021480&w=2 > > I don't think looking at i_size really helps us with that, but I could > be missing other changes in the cow series. The i_size detection isn't new in this series, just slightly moved around. And it really is just intended as an optimization to not even bother if we are beyond i_size. > > In general I'm looking at putting something like this in > xfs_iomap_write_allocate() once the data fork sequence number tracking > is enabled: > > /* > * Now that we have ILOCK we must account for the fact > * that the fork (and thus our mapping) could have > * changed while the inode was unlocked. If the fork > * has changed, trim the caller's mapping to the > * current extent in the fork. We don't even look at the callers mapping except for the range to cover. And that is how e.g. direct I/O also works and a good thing as far as I can tell. To make use of the previous mapping we'd have to rewrite xfs_bmapi_write. If we want to be able to reuse existing mapings I think the sequences are helping us a bit, but a lot more work is needed, and it should be done in a generic way and not just in this path.
