| Message ID | 20190111123032.31538-4-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: properly invalidate cached writeback mapping | expand |
On Fri, Jan 11, 2019 at 07:30:31AM -0500, Brian Foster wrote: > The writeback code caches the current extent mapping across multiple > xfs_do_writepage() calls to avoid repeated lookups for sequential > pages backed by the same extent. This is known to be slightly racy > with extent fork changes in certain difficult to reproduce > scenarios. The cached extent is trimmed to within EOF to help avoid > the most common vector for this problem via speculative > preallocation management, but this is a band-aid that does not > address the fundamental problem. > > Now that we have an xfs_ifork sequence counter mechanism used to > facilitate COW writeback, we can use the same mechanism to validate > consistency between the data fork and cached writeback mappings. On > its face, this is somewhat of a big hammer approach because any > change to the data fork invalidates any mapping currently cached by > a writeback in progress regardless of whether the data fork change > overlaps with the range under writeback. In practice, however, the > impact of this approach is minimal in most cases. > > First, data fork changes (delayed allocations) caused by sustained > sequential buffered writes are amortized across speculative > preallocations. This means that a cached mapping won't be > invalidated by each buffered write of a common file copy workload, > but rather only on less frequent allocation events. Second, the > extent tree is always entirely in-core so an additional lookup of a > usable extent mostly costs a shared ilock cycle and in-memory tree > lookup. This means that a cached mapping reval is relatively cheap > compared to the I/O itself. Third, spurious invalidations don't > impact ioend construction. This means that even if the same extent > is revalidated multiple times across multiple writepage instances, > we still construct and submit the same size ioend (and bio) if the > blocks are physically contiguous. > > Update struct xfs_writepage_ctx with a new field to hold the > sequence number of the data fork associated with the currently > cached mapping. Check the wpc seqno against the data fork when the > mapping is validated and reestablish the mapping whenever the fork > has changed since the mapping was cached. This ensures that > writeback always uses a valid extent mapping and thus prevents lost > writebacks and stale delalloc block problems. > > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/xfs_aops.c | 8 ++++++-- > fs/xfs/xfs_iomap.c | 4 ++-- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > index d9048bcea49c..33a1be5df99f 100644 > --- a/fs/xfs/xfs_aops.c > +++ b/fs/xfs/xfs_aops.c > @@ -29,6 +29,7 @@ > struct xfs_writepage_ctx { > struct xfs_bmbt_irec imap; > unsigned int io_type; > + unsigned int data_seq; > unsigned int cow_seq; > struct xfs_ioend *ioend; > }; > @@ -347,7 +348,8 @@ xfs_map_blocks( > * out that ensures that we always see the current value. > */ > imap_valid = offset_fsb >= wpc->imap.br_startoff && > - offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount; > + offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount && > + wpc->data_seq == READ_ONCE(ip->i_df.if_seq); > if (imap_valid && > (!xfs_inode_has_cow_data(ip) || > wpc->io_type == XFS_IO_COW || I suspect this next "if (imap_valid) ..." logic needs to be updated, too. i.e. the next line is checking if the cow_seq has not changed. i.e. I think wrapping this up in a helper (again!) might make more sense: static bool xfs_imap_valid( struct xfs_inode *ip, struct xfs_writepage_ctx *wpc, xfs_fileoff_t offset_fsb) { if (offset_fsb < wpc->imap.br_startoff) return false; if (offset_fsb >= wpc->imap.br_startoff + wpc->imap.br_blockcount) return false; if (wpc->data_seq != READ_ONCE(ip->i_df.if_seq) return false; if (!xfs_inode_has_cow_data(ip)) return true; if (wpc->io_type != XFS_IO_COW) return true; if (wpc->cow_seq != READ_ONCE(ip->i_cowfp->if_seq) return false; return true; } and then put the shutdown check before we check the map for validity (i.e. don't continue to write to the cached map after a shutdown has been triggered): if (XFS_FORCED_SHUTDOWN(mp)) return -EIO; if (xfs_imap_valid(ip, wpc, offset_fsb)) return 0; > @@ -417,6 +419,7 @@ xfs_map_blocks( > */ > if (!xfs_iext_lookup_extent(ip, &ip->i_df, offset_fsb, &icur, &imap)) > imap.br_startoff = end_fsb; /* fake a hole past EOF */ > + wpc->data_seq = READ_ONCE(ip->i_df.if_seq); > xfs_iunlock(ip, XFS_ILOCK_SHARED); > > if (imap.br_startoff > offset_fsb) { > @@ -454,7 +457,8 @@ xfs_map_blocks( > return 0; > allocate_blocks: > error = xfs_iomap_write_allocate(ip, whichfork, offset, &imap, > - &wpc->cow_seq); > + whichfork == XFS_COW_FORK ? > + &wpc->cow_seq : &wpc->data_seq); > if (error) > return error; > ASSERT(whichfork == XFS_COW_FORK || cow_fsb == NULLFILEOFF || > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c > index 27c93b5f029d..0401e33d4e8f 100644 > --- a/fs/xfs/xfs_iomap.c > +++ b/fs/xfs/xfs_iomap.c > @@ -681,7 +681,7 @@ xfs_iomap_write_allocate( > int whichfork, > xfs_off_t offset, > xfs_bmbt_irec_t *imap, > - unsigned int *cow_seq) > + unsigned int *seq) > { > xfs_mount_t *mp = ip->i_mount; > struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); > @@ -798,7 +798,7 @@ xfs_iomap_write_allocate( > goto error0; > > if (whichfork == XFS_COW_FORK) > - *cow_seq = READ_ONCE(ifp->if_seq); > + *seq = READ_ONCE(ifp->if_seq); > xfs_iunlock(ip, XFS_ILOCK_EXCL); > } One of the things that limits xfs_iomap_write_allocate() efficiency is the mitigations for races against truncate. i.e. the huge comment that starts: /* * it is possible that the extents have changed since * we did the read call as we dropped the ilock for a * while. We have to be careful about truncates or hole * punchs here - we are not allowed to allocate * non-delalloc blocks here. .... Now that we can detect that the extents have changed in the data fork, we can go back to allocating multiple extents per xfs_bmapi_write() call by doing a sequence number check after we lock the inode. If the sequence number does not match what was passed in or returned from the previous loop, we return -EAGAIN. Hmmm, looking at the existing -EAGAIN case, I suspect this isn't handled correctly by xfs_map_blocks() anymore. i.e. it just returns the error which can lead to discarding the page rather than checking to see if the there was a valid map allocated. I think there's some followup work here (another patch series). :/ Cheers, Dave.
On Mon, Jan 14, 2019 at 08:49:05AM +1100, Dave Chinner wrote: > On Fri, Jan 11, 2019 at 07:30:31AM -0500, Brian Foster wrote: > > The writeback code caches the current extent mapping across multiple > > xfs_do_writepage() calls to avoid repeated lookups for sequential > > pages backed by the same extent. This is known to be slightly racy > > with extent fork changes in certain difficult to reproduce > > scenarios. The cached extent is trimmed to within EOF to help avoid > > the most common vector for this problem via speculative > > preallocation management, but this is a band-aid that does not > > address the fundamental problem. > > > > Now that we have an xfs_ifork sequence counter mechanism used to > > facilitate COW writeback, we can use the same mechanism to validate > > consistency between the data fork and cached writeback mappings. On > > its face, this is somewhat of a big hammer approach because any > > change to the data fork invalidates any mapping currently cached by > > a writeback in progress regardless of whether the data fork change > > overlaps with the range under writeback. In practice, however, the > > impact of this approach is minimal in most cases. > > > > First, data fork changes (delayed allocations) caused by sustained > > sequential buffered writes are amortized across speculative > > preallocations. This means that a cached mapping won't be > > invalidated by each buffered write of a common file copy workload, > > but rather only on less frequent allocation events. Second, the > > extent tree is always entirely in-core so an additional lookup of a > > usable extent mostly costs a shared ilock cycle and in-memory tree > > lookup. This means that a cached mapping reval is relatively cheap > > compared to the I/O itself. Third, spurious invalidations don't > > impact ioend construction. This means that even if the same extent > > is revalidated multiple times across multiple writepage instances, > > we still construct and submit the same size ioend (and bio) if the > > blocks are physically contiguous. > > > > Update struct xfs_writepage_ctx with a new field to hold the > > sequence number of the data fork associated with the currently > > cached mapping. Check the wpc seqno against the data fork when the > > mapping is validated and reestablish the mapping whenever the fork > > has changed since the mapping was cached. This ensures that > > writeback always uses a valid extent mapping and thus prevents lost > > writebacks and stale delalloc block problems. > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > --- > > fs/xfs/xfs_aops.c | 8 ++++++-- > > fs/xfs/xfs_iomap.c | 4 ++-- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > > index d9048bcea49c..33a1be5df99f 100644 > > --- a/fs/xfs/xfs_aops.c > > +++ b/fs/xfs/xfs_aops.c > > @@ -29,6 +29,7 @@ > > struct xfs_writepage_ctx { > > struct xfs_bmbt_irec imap; > > unsigned int io_type; > > + unsigned int data_seq; > > unsigned int cow_seq; > > struct xfs_ioend *ioend; > > }; > > @@ -347,7 +348,8 @@ xfs_map_blocks( > > * out that ensures that we always see the current value. > > */ > > imap_valid = offset_fsb >= wpc->imap.br_startoff && > > - offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount; > > + offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount && > > + wpc->data_seq == READ_ONCE(ip->i_df.if_seq); > > if (imap_valid && > > (!xfs_inode_has_cow_data(ip) || > > wpc->io_type == XFS_IO_COW || > > I suspect this next "if (imap_valid) ..." logic needs to be updated, > too. i.e. the next line is checking if the cow_seq has not changed. > I'm not quite sure what you're getting at here. By "next," do you mean the one you've quoted or the post-lock cycle check (a re-check at the latter point makes sense to me). Otherwise the imap check is intentionally distinct from the COW seq check because these control independent bits of subsequent logic (in certain cases). That said, now that I look at it again this logic is rather convoluted because imap_valid doesn't necessarily refer to the data fork (e.g., if ->imap is a cow fork extent). So yeah, this all should probably be refactored... > i.e. I think wrapping this up in a helper (again!) might make more > sense: > > static bool > xfs_imap_valid( > struct xfs_inode *ip, > struct xfs_writepage_ctx *wpc, > xfs_fileoff_t offset_fsb) > { > if (offset_fsb < wpc->imap.br_startoff) > return false; > if (offset_fsb >= wpc->imap.br_startoff + wpc->imap.br_blockcount) > return false; > if (wpc->data_seq != READ_ONCE(ip->i_df.if_seq) > return false; > if (!xfs_inode_has_cow_data(ip)) > return true; > if (wpc->io_type != XFS_IO_COW) > return true; > if (wpc->cow_seq != READ_ONCE(ip->i_cowfp->if_seq) > return false; > return true; > } > I think you mean 'if (io_type == XFS_IO_COW)'? Otherwise this seems reasonable, though I think the logic suffers a bit from the same problem as above. How about with the following tweaks (and comments to try and make this easier to follow)? static bool xfs_imap_valid() { if (offset_fsb < wpc->imap.br_startoff) return false; if (offset_fsb >= wpc->imap.br_startoff + wpc->imap.br_blockcount) return false; /* a valid range is sufficient for COW mappings */ if (wpc->io_type == XFS_IO_COW) return true; /* * Not a COW mapping. Revalidate across changes in either the * data or COW fork ... */ if (wpc->data_seq != READ_ONCE(ip->i_df.if_seq) return false; if (xfs_inode_has_cow_data(ip) && wpc->cow_seq != READ_ONCE(ip->i_cowfp->if_seq) return false; return true; } I think that technically we could skip the == XFS_IO_COW check and we'd just be more conservative by essentially applying the same fork change logic we are for the data fork, but that's not really the intent of this patch. > and then put the shutdown check before we check the map for validity > (i.e. don't continue to write to the cached map after a shutdown has > been triggered): > Ack. > if (XFS_FORCED_SHUTDOWN(mp)) > return -EIO; > > if (xfs_imap_valid(ip, wpc, offset_fsb)) > return 0; > > > > @@ -417,6 +419,7 @@ xfs_map_blocks( > > */ > > if (!xfs_iext_lookup_extent(ip, &ip->i_df, offset_fsb, &icur, &imap)) > > imap.br_startoff = end_fsb; /* fake a hole past EOF */ > > + wpc->data_seq = READ_ONCE(ip->i_df.if_seq); > > xfs_iunlock(ip, XFS_ILOCK_SHARED); > > > > if (imap.br_startoff > offset_fsb) { > > @@ -454,7 +457,8 @@ xfs_map_blocks( > > return 0; > > allocate_blocks: > > error = xfs_iomap_write_allocate(ip, whichfork, offset, &imap, > > - &wpc->cow_seq); > > + whichfork == XFS_COW_FORK ? > > + &wpc->cow_seq : &wpc->data_seq); > > if (error) > > return error; > > ASSERT(whichfork == XFS_COW_FORK || cow_fsb == NULLFILEOFF || > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c > > index 27c93b5f029d..0401e33d4e8f 100644 > > --- a/fs/xfs/xfs_iomap.c > > +++ b/fs/xfs/xfs_iomap.c > > @@ -681,7 +681,7 @@ xfs_iomap_write_allocate( > > int whichfork, > > xfs_off_t offset, > > xfs_bmbt_irec_t *imap, > > - unsigned int *cow_seq) > > + unsigned int *seq) > > { > > xfs_mount_t *mp = ip->i_mount; > > struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); > > @@ -798,7 +798,7 @@ xfs_iomap_write_allocate( > > goto error0; > > > > if (whichfork == XFS_COW_FORK) > > - *cow_seq = READ_ONCE(ifp->if_seq); > > + *seq = READ_ONCE(ifp->if_seq); > > xfs_iunlock(ip, XFS_ILOCK_EXCL); > > } > > One of the things that limits xfs_iomap_write_allocate() efficiency > is the mitigations for races against truncate. i.e. the huge comment that > starts: > > /* > * it is possible that the extents have changed since > * we did the read call as we dropped the ilock for a > * while. We have to be careful about truncates or hole > * punchs here - we are not allowed to allocate > * non-delalloc blocks here. > .... > Hmm, Ok... so this fix goes a ways back to commit e4143a1cf5 ("[XFS] Fix transaction overrun during writeback."). It sounds like the issue was an instance of the "attempt to convert delalloc blocks ends up doing physical allocation" problem (which results in a transaction overrun). > Now that we can detect that the extents have changed in the data > fork, we can go back to allocating multiple extents per > xfs_bmapi_write() call by doing a sequence number check after we > lock the inode. If the sequence number does not match what was > passed in or returned from the previous loop, we return -EAGAIN. > I'm not familiar with this particular instance of this problem (we've certainly had other instances of the same thing), but the surrounding context of this code has changed quite a bit. Most notably is XFS_BMAPI_DELALLOC, which was intended to mitigate this problem by disallowing real allocation in such calls. > Hmmm, looking at the existing -EAGAIN case, I suspect this isn't > handled correctly by xfs_map_blocks() anymore. i.e. it just returns > the error which can lead to discarding the page rather than checking > to see if the there was a valid map allocated. I think there's some > followup work here (another patch series). :/ > Ok. At the moment, that error looks like it should only happen if we're past EOF..? Either way, the XFS_BMAPI_DELALLOC thing still can result in an error so it probably makes sense to tie a seqno check to -EAGAIN and handle it properly in the caller. Hmm, given that we can really only handle one extent at a time up through the caller (as also noted in the big comment you quoted) and that this series introduces more aggressive revalidation as it is, I am wondering what real value there is in doing more delalloc conversions here than technically required. ISTM that removing some of this i_size checking code and doing the seqno based kickback may actually be cleaner. I'll need to have a closer look from an optimization perspective when the correctness issues are dealt with. I also could have sworn I removed that whichfork check from xfs_iomap_write_allocate(), but apparently not... ;P Brian > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Mon, Jan 14, 2019 at 10:34:23AM -0500, Brian Foster wrote: > On Mon, Jan 14, 2019 at 08:49:05AM +1100, Dave Chinner wrote: > > On Fri, Jan 11, 2019 at 07:30:31AM -0500, Brian Foster wrote: > > > The writeback code caches the current extent mapping across multiple > > > xfs_do_writepage() calls to avoid repeated lookups for sequential > > > pages backed by the same extent. This is known to be slightly racy > > > with extent fork changes in certain difficult to reproduce > > > scenarios. The cached extent is trimmed to within EOF to help avoid > > > the most common vector for this problem via speculative > > > preallocation management, but this is a band-aid that does not > > > address the fundamental problem. > > > > > > Now that we have an xfs_ifork sequence counter mechanism used to > > > facilitate COW writeback, we can use the same mechanism to validate > > > consistency between the data fork and cached writeback mappings. On > > > its face, this is somewhat of a big hammer approach because any > > > change to the data fork invalidates any mapping currently cached by > > > a writeback in progress regardless of whether the data fork change > > > overlaps with the range under writeback. In practice, however, the > > > impact of this approach is minimal in most cases. > > > > > > First, data fork changes (delayed allocations) caused by sustained > > > sequential buffered writes are amortized across speculative > > > preallocations. This means that a cached mapping won't be > > > invalidated by each buffered write of a common file copy workload, > > > but rather only on less frequent allocation events. Second, the > > > extent tree is always entirely in-core so an additional lookup of a > > > usable extent mostly costs a shared ilock cycle and in-memory tree > > > lookup. This means that a cached mapping reval is relatively cheap > > > compared to the I/O itself. Third, spurious invalidations don't > > > impact ioend construction. This means that even if the same extent > > > is revalidated multiple times across multiple writepage instances, > > > we still construct and submit the same size ioend (and bio) if the > > > blocks are physically contiguous. > > > > > > Update struct xfs_writepage_ctx with a new field to hold the > > > sequence number of the data fork associated with the currently > > > cached mapping. Check the wpc seqno against the data fork when the > > > mapping is validated and reestablish the mapping whenever the fork > > > has changed since the mapping was cached. This ensures that > > > writeback always uses a valid extent mapping and thus prevents lost > > > writebacks and stale delalloc block problems. > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > --- > > > fs/xfs/xfs_aops.c | 8 ++++++-- > > > fs/xfs/xfs_iomap.c | 4 ++-- > > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > > > index d9048bcea49c..33a1be5df99f 100644 > > > --- a/fs/xfs/xfs_aops.c > > > +++ b/fs/xfs/xfs_aops.c > > > @@ -29,6 +29,7 @@ > > > struct xfs_writepage_ctx { > > > struct xfs_bmbt_irec imap; > > > unsigned int io_type; > > > + unsigned int data_seq; > > > unsigned int cow_seq; > > > struct xfs_ioend *ioend; > > > }; > > > @@ -347,7 +348,8 @@ xfs_map_blocks( > > > * out that ensures that we always see the current value. > > > */ > > > imap_valid = offset_fsb >= wpc->imap.br_startoff && > > > - offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount; > > > + offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount && > > > + wpc->data_seq == READ_ONCE(ip->i_df.if_seq); > > > if (imap_valid && > > > (!xfs_inode_has_cow_data(ip) || > > > wpc->io_type == XFS_IO_COW || > > > > I suspect this next "if (imap_valid) ..." logic needs to be updated, > > too. i.e. the next line is checking if the cow_seq has not changed. > > > > I'm not quite sure what you're getting at here. By "next," do you mean > the one you've quoted or the post-lock cycle check (a re-check at the > latter point makes sense to me). Otherwise the imap check is > intentionally distinct from the COW seq check because these control > independent bits of subsequent logic (in certain cases). No, I meant the next line of code that isn't in the hunk was: if (imap_valid && (!xfs_inode_has_cow_data(ip) || wpc->io_type == XFS_IO_COW || >>>>>> wpc->cow_seq != READ_ONCE(ip->i_cowfp->if_seq)) The cow fork sequence number check. > I think you mean 'if (io_type == XFS_IO_COW)'? Otherwise this seems > reasonable, though I think the logic suffers a bit from the same problem > as above. How about with the following tweaks (and comments to try and > make this easier to follow)? I misread the nested () and so got the new logic wrong. :) > static bool > xfs_imap_valid() > { > if (offset_fsb < wpc->imap.br_startoff) > return false; > if (offset_fsb >= wpc->imap.br_startoff + wpc->imap.br_blockcount) > return false; > /* a valid range is sufficient for COW mappings */ > if (wpc->io_type == XFS_IO_COW) > return true; > > /* > * Not a COW mapping. Revalidate across changes in either the > * data or COW fork ... > */ > if (wpc->data_seq != READ_ONCE(ip->i_df.if_seq) > return false; > if (xfs_inode_has_cow_data(ip) && > wpc->cow_seq != READ_ONCE(ip->i_cowfp->if_seq) > return false; > > return true; > } Yup, that's what I meant. I'm glad you're on the ball right now :) > I think that technically we could skip the == XFS_IO_COW check and we'd > just be more conservative by essentially applying the same fork change > logic we are for the data fork, but that's not really the intent of this > patch. Sure. > > > xfs_mount_t *mp = ip->i_mount; > > > struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); > > > @@ -798,7 +798,7 @@ xfs_iomap_write_allocate( > > > goto error0; > > > > > > if (whichfork == XFS_COW_FORK) > > > - *cow_seq = READ_ONCE(ifp->if_seq); > > > + *seq = READ_ONCE(ifp->if_seq); > > > xfs_iunlock(ip, XFS_ILOCK_EXCL); > > > } > > > > One of the things that limits xfs_iomap_write_allocate() efficiency > > is the mitigations for races against truncate. i.e. the huge comment that > > starts: > > > > /* > > * it is possible that the extents have changed since > > * we did the read call as we dropped the ilock for a > > * while. We have to be careful about truncates or hole > > * punchs here - we are not allowed to allocate > > * non-delalloc blocks here. > > .... > > > > Hmm, Ok... so this fix goes a ways back to commit e4143a1cf5 ("[XFS] Fix > transaction overrun during writeback."). It sounds like the issue was an > instance of the "attempt to convert delalloc blocks ends up doing > physical allocation" problem (which results in a transaction overrun). Yeah, there were no delalloc blocks because they'd been truncated or punched away between unlock/lock cycles on the inode. > > Now that we can detect that the extents have changed in the data > > fork, we can go back to allocating multiple extents per > > xfs_bmapi_write() call by doing a sequence number check after we > > lock the inode. If the sequence number does not match what was > > passed in or returned from the previous loop, we return -EAGAIN. > > > > I'm not familiar with this particular instance of this problem (we've > certainly had other instances of the same thing), but the surrounding > context of this code has changed quite a bit. Yes, it has. The move to a single map was done a long time ago because there weren't any other options at the time, and it was a problem we'd been struggling to understand and sort out for years. > Most notably is > XFS_BMAPI_DELALLOC, which was intended to mitigate this problem by > disallowing real allocation in such calls. Yup. however, I've always thought of it as a bit of a hack - it's preventing the transaction overrun when a problem occurs as opposed to preventing the race that leads to trying to allocate over a hole. Essentially, though they are both trying to address the same problem: that the extent list can change during writeback and writeback ends up using stale information to direct IO and/or extent allocation. > > Hmmm, looking at the existing -EAGAIN case, I suspect this isn't > > handled correctly by xfs_map_blocks() anymore. i.e. it just returns > > the error which can lead to discarding the page rather than checking > > to see if the there was a valid map allocated. I think there's some > > followup work here (another patch series). :/ > > > > Ok. At the moment, that error looks like it should only happen if we're > past EOF..? Yeah, racing with truncate. The old writeback code used to have a non-blocking feature which would handle -EAGAIN errors bubbling up from anywhere in the writeback path. We got rid of that a long time ago, so I suspect this has been broken for a long while. > Either way, the XFS_BMAPI_DELALLOC thing still can result in > an error so it probably makes sense to tie a seqno check to -EAGAIN and > handle it properly in the caller. *nod* > Hmm, given that we can really only handle one extent at a time up > through the caller (as also noted in the big comment you quoted) and > that this series introduces more aggressive revalidation as it is, I am > wondering what real value there is in doing more delalloc conversions > here than technically required. When the filesystem gets fragmented and there isn't a large enough free space to allocate over the delalloc extent, it was more CPU efficient to allocate multiple extents in a single xfs_bmapi_write() call and transaction, similar to how we can free 2 extents in a single truncate transaction. We still do this in xfs_da_grow_inode_int() using nmaps = XFS_BMAP_MAX_NMAP (i.e. 4) so the code should still work if we were to pass it multiple maps. But, yes, the code is very different now, so it may not make sense to attempt multiple extent allocation here again. > ISTM that removing some of this i_size > checking code and doing the seqno based kickback may actually be > cleaner. I'll need to have a closer look from an optimization > perspective when the correctness issues are dealt with. > > I also could have sworn I removed that whichfork check from > xfs_iomap_write_allocate(), but apparently not... ;P Maybe it got blown into a dusty corner when we weren't paying attention. :) Cheers, dave.
On Tue, Jan 15, 2019 at 07:57:04AM +1100, Dave Chinner wrote: > On Mon, Jan 14, 2019 at 10:34:23AM -0500, Brian Foster wrote: > > On Mon, Jan 14, 2019 at 08:49:05AM +1100, Dave Chinner wrote: > > > On Fri, Jan 11, 2019 at 07:30:31AM -0500, Brian Foster wrote: > > > > The writeback code caches the current extent mapping across multiple > > > > xfs_do_writepage() calls to avoid repeated lookups for sequential > > > > pages backed by the same extent. This is known to be slightly racy > > > > with extent fork changes in certain difficult to reproduce > > > > scenarios. The cached extent is trimmed to within EOF to help avoid > > > > the most common vector for this problem via speculative > > > > preallocation management, but this is a band-aid that does not > > > > address the fundamental problem. > > > > > > > > Now that we have an xfs_ifork sequence counter mechanism used to > > > > facilitate COW writeback, we can use the same mechanism to validate > > > > consistency between the data fork and cached writeback mappings. On > > > > its face, this is somewhat of a big hammer approach because any > > > > change to the data fork invalidates any mapping currently cached by > > > > a writeback in progress regardless of whether the data fork change > > > > overlaps with the range under writeback. In practice, however, the > > > > impact of this approach is minimal in most cases. > > > > > > > > First, data fork changes (delayed allocations) caused by sustained > > > > sequential buffered writes are amortized across speculative > > > > preallocations. This means that a cached mapping won't be > > > > invalidated by each buffered write of a common file copy workload, > > > > but rather only on less frequent allocation events. Second, the > > > > extent tree is always entirely in-core so an additional lookup of a > > > > usable extent mostly costs a shared ilock cycle and in-memory tree > > > > lookup. This means that a cached mapping reval is relatively cheap > > > > compared to the I/O itself. Third, spurious invalidations don't > > > > impact ioend construction. This means that even if the same extent > > > > is revalidated multiple times across multiple writepage instances, > > > > we still construct and submit the same size ioend (and bio) if the > > > > blocks are physically contiguous. > > > > > > > > Update struct xfs_writepage_ctx with a new field to hold the > > > > sequence number of the data fork associated with the currently > > > > cached mapping. Check the wpc seqno against the data fork when the > > > > mapping is validated and reestablish the mapping whenever the fork > > > > has changed since the mapping was cached. This ensures that > > > > writeback always uses a valid extent mapping and thus prevents lost > > > > writebacks and stale delalloc block problems. > > > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > > --- > > > > fs/xfs/xfs_aops.c | 8 ++++++-- > > > > fs/xfs/xfs_iomap.c | 4 ++-- > > > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > > > > index d9048bcea49c..33a1be5df99f 100644 > > > > --- a/fs/xfs/xfs_aops.c > > > > +++ b/fs/xfs/xfs_aops.c > > > > @@ -29,6 +29,7 @@ > > > > struct xfs_writepage_ctx { > > > > struct xfs_bmbt_irec imap; > > > > unsigned int io_type; > > > > + unsigned int data_seq; > > > > unsigned int cow_seq; > > > > struct xfs_ioend *ioend; > > > > }; > > > > @@ -347,7 +348,8 @@ xfs_map_blocks( > > > > * out that ensures that we always see the current value. > > > > */ > > > > imap_valid = offset_fsb >= wpc->imap.br_startoff && > > > > - offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount; > > > > + offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount && > > > > + wpc->data_seq == READ_ONCE(ip->i_df.if_seq); > > > > if (imap_valid && > > > > (!xfs_inode_has_cow_data(ip) || > > > > wpc->io_type == XFS_IO_COW || > > > > > > I suspect this next "if (imap_valid) ..." logic needs to be updated, > > > too. i.e. the next line is checking if the cow_seq has not changed. > > > > > > > I'm not quite sure what you're getting at here. By "next," do you mean > > the one you've quoted or the post-lock cycle check (a re-check at the > > latter point makes sense to me). Otherwise the imap check is > > intentionally distinct from the COW seq check because these control > > independent bits of subsequent logic (in certain cases). > > No, I meant the next line of code that isn't in the hunk was: > > if (imap_valid && > (!xfs_inode_has_cow_data(ip) || > wpc->io_type == XFS_IO_COW || > >>>>>> wpc->cow_seq != READ_ONCE(ip->i_cowfp->if_seq)) > > The cow fork sequence number check. > > > I think you mean 'if (io_type == XFS_IO_COW)'? Otherwise this seems > > reasonable, though I think the logic suffers a bit from the same problem > > as above. How about with the following tweaks (and comments to try and > > make this easier to follow)? > > I misread the nested () and so got the new logic wrong. :) > Oh, Ok. Well I'm planning to use the helper and issue another xfs_imap_valid() call as described either way. I think this is more appropriate for clarity and because imap_valid in this v1 includes the ->if_seq check and the latter can change across the lock cycle. > > static bool > > xfs_imap_valid() > > { > > if (offset_fsb < wpc->imap.br_startoff) > > return false; > > if (offset_fsb >= wpc->imap.br_startoff + wpc->imap.br_blockcount) > > return false; > > /* a valid range is sufficient for COW mappings */ > > if (wpc->io_type == XFS_IO_COW) > > return true; > > > > /* > > * Not a COW mapping. Revalidate across changes in either the > > * data or COW fork ... > > */ > > if (wpc->data_seq != READ_ONCE(ip->i_df.if_seq) > > return false; > > if (xfs_inode_has_cow_data(ip) && > > wpc->cow_seq != READ_ONCE(ip->i_cowfp->if_seq) > > return false; > > > > return true; > > } > > Yup, that's what I meant. I'm glad you're on the ball right now :) > > > I think that technically we could skip the == XFS_IO_COW check and we'd > > just be more conservative by essentially applying the same fork change > > logic we are for the data fork, but that's not really the intent of this > > patch. > > Sure. > > > > > xfs_mount_t *mp = ip->i_mount; > > > > struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); > > > > @@ -798,7 +798,7 @@ xfs_iomap_write_allocate( > > > > goto error0; > > > > > > > > if (whichfork == XFS_COW_FORK) > > > > - *cow_seq = READ_ONCE(ifp->if_seq); > > > > + *seq = READ_ONCE(ifp->if_seq); > > > > xfs_iunlock(ip, XFS_ILOCK_EXCL); > > > > } > > > > > > One of the things that limits xfs_iomap_write_allocate() efficiency > > > is the mitigations for races against truncate. i.e. the huge comment that > > > starts: > > > > > > /* > > > * it is possible that the extents have changed since > > > * we did the read call as we dropped the ilock for a > > > * while. We have to be careful about truncates or hole > > > * punchs here - we are not allowed to allocate > > > * non-delalloc blocks here. > > > .... > > > > > > > Hmm, Ok... so this fix goes a ways back to commit e4143a1cf5 ("[XFS] Fix > > transaction overrun during writeback."). It sounds like the issue was an > > instance of the "attempt to convert delalloc blocks ends up doing > > physical allocation" problem (which results in a transaction overrun). > > Yeah, there were no delalloc blocks because they'd been truncated or > punched away between unlock/lock cycles on the inode. > > > > Now that we can detect that the extents have changed in the data > > > fork, we can go back to allocating multiple extents per > > > xfs_bmapi_write() call by doing a sequence number check after we > > > lock the inode. If the sequence number does not match what was > > > passed in or returned from the previous loop, we return -EAGAIN. > > > > > > > I'm not familiar with this particular instance of this problem (we've > > certainly had other instances of the same thing), but the surrounding > > context of this code has changed quite a bit. > > Yes, it has. The move to a single map was done a long time ago > because there weren't any other options at the time, and it was a > problem we'd been struggling to understand and sort out for years. > > > Most notably is > > XFS_BMAPI_DELALLOC, which was intended to mitigate this problem by > > disallowing real allocation in such calls. > > Yup. however, I've always thought of it as a bit of a hack - it's > preventing the transaction overrun when a problem occurs as opposed > to preventing the race that leads to trying to allocate over a > hole. > > Essentially, though they are both trying to address the same > problem: that the extent list can change during writeback and > writeback ends up using stale information to direct IO and/or extent > allocation. > Fair point. > > > Hmmm, looking at the existing -EAGAIN case, I suspect this isn't > > > handled correctly by xfs_map_blocks() anymore. i.e. it just returns > > > the error which can lead to discarding the page rather than checking > > > to see if the there was a valid map allocated. I think there's some > > > followup work here (another patch series). :/ > > > > > > > Ok. At the moment, that error looks like it should only happen if we're > > past EOF..? > > Yeah, racing with truncate. The old writeback code used to have a > non-blocking feature which would handle -EAGAIN errors bubbling up > from anywhere in the writeback path. We got rid of that a long time > ago, so I suspect this has been broken for a long while. > > > Either way, the XFS_BMAPI_DELALLOC thing still can result in > > an error so it probably makes sense to tie a seqno check to -EAGAIN and > > handle it properly in the caller. > > *nod* > After taking a closer look at this, one thing that concerns me about just sticking an ->if_seq check in xfs_iomap_write_allocate() is the potential to bounce back and forth between xfs_iomap_write_allocate() and the caller due to the fact that ->if_seq changes on any change in the fork. If we just return -EAGAIN and retry, then some other task can cause writeback churn by just punching/reallocating a block somewhere else in the file while this code repeats lookups of the same extent. I think the fact that we hold the page lock across these ilock cycles means we should at minimum be able to rely on stability of the blocks backing the current page. I.e. if we're in xfs_iomap_write_allocate(), we've found a delalloc extent behind the page while under page lock. Truncate and hole punch both call into truncate_pagecache_range(), which locks every page and waits on writeback before either is allowed to do any block manipulation. Given that, I'm thinking of doing something like look up the extent that covers offset_fsb on an ->if_seq change and trim the passed in extent (i.e. mapping range) to whatever sits in the extent tree. That means we preserve validity of the mapping without risk of disruption due to unrelated changes in the fork. We also no longer implicitly/hackily rely on XFS_IO_DELALLOC to sanitize the mapping range passed into xfs_bmapi_write() and so should only ever expect an error if we truly screw something up. I think the subtle tradeoff vs. a high level retry is that we'd do writeback to the current page rather than back off at the last second and redirty the page if a truncate was about to kill it off as we're processing it for writeback. As noted above, page truncation still has to wait on page writeback so I don't think that should be a correctness issue. I still need to hack/test on this a bit to determine whether this is sane, but if the code ends up more simple I think that might be a reasonable tradeoff.. Brian > > Hmm, given that we can really only handle one extent at a time up > > through the caller (as also noted in the big comment you quoted) and > > that this series introduces more aggressive revalidation as it is, I am > > wondering what real value there is in doing more delalloc conversions > > here than technically required. > > When the filesystem gets fragmented and there isn't a large enough > free space to allocate over the delalloc extent, it was more CPU > efficient to allocate multiple extents in a single xfs_bmapi_write() > call and transaction, similar to how we can free 2 extents in a > single truncate transaction. > > We still do this in xfs_da_grow_inode_int() using nmaps = > XFS_BMAP_MAX_NMAP (i.e. 4) so the code should still work if we were > to pass it multiple maps. But, yes, the code is very different now, > so it may not make sense to attempt multiple extent allocation here > again. > > > ISTM that removing some of this i_size > > checking code and doing the seqno based kickback may actually be > > cleaner. I'll need to have a closer look from an optimization > > perspective when the correctness issues are dealt with. > > > > I also could have sworn I removed that whichfork check from > > xfs_iomap_write_allocate(), but apparently not... ;P > > Maybe it got blown into a dusty corner when we weren't paying > attention. :) > > Cheers, > > dave. > -- > Dave Chinner > david@fromorbit.com
On Mon, Jan 14, 2019 at 10:34:23AM -0500, Brian Foster wrote: > static bool > xfs_imap_valid() > { > if (offset_fsb < wpc->imap.br_startoff) > return false; > if (offset_fsb >= wpc->imap.br_startoff + wpc->imap.br_blockcount) > return false; > /* a valid range is sufficient for COW mappings */ > if (wpc->io_type == XFS_IO_COW) > return true; > > /* > * Not a COW mapping. Revalidate across changes in either the > * data or COW fork ... > */ > if (wpc->data_seq != READ_ONCE(ip->i_df.if_seq) > return false; > if (xfs_inode_has_cow_data(ip) && > wpc->cow_seq != READ_ONCE(ip->i_cowfp->if_seq) > return false; > > return true; > } > > I think that technically we could skip the == XFS_IO_COW check and we'd > just be more conservative by essentially applying the same fork change > logic we are for the data fork, but that's not really the intent of this > patch. That above logic looks pretty sensible to me. And I don't think there is any need for being more conservative. > > One of the things that limits xfs_iomap_write_allocate() efficiency > > is the mitigations for races against truncate. i.e. the huge comment that > > starts: > > > > /* > > * it is possible that the extents have changed since > > * we did the read call as we dropped the ilock for a > > * while. We have to be careful about truncates or hole > > * punchs here - we are not allowed to allocate > > * non-delalloc blocks here. > > .... > > > > Hmm, Ok... so this fix goes a ways back to commit e4143a1cf5 ("[XFS] Fix > transaction overrun during writeback."). It sounds like the issue was an > instance of the "attempt to convert delalloc blocks ends up doing > physical allocation" problem (which results in a transaction overrun). FYI, that area is touched by my always COW series, it would be great if I could get another review for that. And yes, I need to dust it off and resende based on the comments from Darrick. I just need to find out how to best combine it with your current series. > > Now that we can detect that the extents have changed in the data > > fork, we can go back to allocating multiple extents per > > xfs_bmapi_write() call by doing a sequence number check after we > > lock the inode. If the sequence number does not match what was > > passed in or returned from the previous loop, we return -EAGAIN. > > > > I'm not familiar with this particular instance of this problem (we've > certainly had other instances of the same thing), but the surrounding > context of this code has changed quite a bit. Most notably is > XFS_BMAPI_DELALLOC, which was intended to mitigate this problem by > disallowing real allocation in such calls. I'm also not sure what doing multiple allocations in one calls is supposed to really buys us. We basically have to roll transactions and redo all checks anyway. > > Hmmm, looking at the existing -EAGAIN case, I suspect this isn't > > handled correctly by xfs_map_blocks() anymore. i.e. it just returns > > the error which can lead to discarding the page rather than checking > > to see if the there was a valid map allocated. I think there's some > > followup work here (another patch series). :/ > > > > Ok. At the moment, that error looks like it should only happen if we're > past EOF..? Either way, the XFS_BMAPI_DELALLOC thing still can result in > an error so it probably makes sense to tie a seqno check to -EAGAIN and > handle it properly in the caller. For that whole -EAGAIN handling please look at my always cow series again, I got bitten by it a few times and also think the current code works only by chance and in the right phase of the moon. I hope the series documents what we had it for very nicely.
On Thu, Jan 17, 2019 at 06:47:28AM -0800, Christoph Hellwig wrote: > On Mon, Jan 14, 2019 at 10:34:23AM -0500, Brian Foster wrote: > > static bool > > xfs_imap_valid() > > { > > if (offset_fsb < wpc->imap.br_startoff) > > return false; > > if (offset_fsb >= wpc->imap.br_startoff + wpc->imap.br_blockcount) > > return false; > > /* a valid range is sufficient for COW mappings */ > > if (wpc->io_type == XFS_IO_COW) > > return true; > > > > /* > > * Not a COW mapping. Revalidate across changes in either the > > * data or COW fork ... > > */ > > if (wpc->data_seq != READ_ONCE(ip->i_df.if_seq) > > return false; > > if (xfs_inode_has_cow_data(ip) && > > wpc->cow_seq != READ_ONCE(ip->i_cowfp->if_seq) > > return false; > > > > return true; > > } > > > > I think that technically we could skip the == XFS_IO_COW check and we'd > > just be more conservative by essentially applying the same fork change > > logic we are for the data fork, but that's not really the intent of this > > patch. > > That above logic looks pretty sensible to me. And I don't think there > is any need for being more conservative. > Agreed. > > > One of the things that limits xfs_iomap_write_allocate() efficiency > > > is the mitigations for races against truncate. i.e. the huge comment that > > > starts: > > > > > > /* > > > * it is possible that the extents have changed since > > > * we did the read call as we dropped the ilock for a > > > * while. We have to be careful about truncates or hole > > > * punchs here - we are not allowed to allocate > > > * non-delalloc blocks here. > > > .... > > > > > > > Hmm, Ok... so this fix goes a ways back to commit e4143a1cf5 ("[XFS] Fix > > transaction overrun during writeback."). It sounds like the issue was an > > instance of the "attempt to convert delalloc blocks ends up doing > > physical allocation" problem (which results in a transaction overrun). > > FYI, that area is touched by my always COW series, it would be great > if I could get another review for that. And yes, I need to dust it off > and resende based on the comments from Darrick. I just need to find > out how to best combine it with your current series. > > > > Now that we can detect that the extents have changed in the data > > > fork, we can go back to allocating multiple extents per > > > xfs_bmapi_write() call by doing a sequence number check after we > > > lock the inode. If the sequence number does not match what was > > > passed in or returned from the previous loop, we return -EAGAIN. > > > > > > > I'm not familiar with this particular instance of this problem (we've > > certainly had other instances of the same thing), but the surrounding > > context of this code has changed quite a bit. Most notably is > > XFS_BMAPI_DELALLOC, which was intended to mitigate this problem by > > disallowing real allocation in such calls. > > I'm also not sure what doing multiple allocations in one calls is > supposed to really buys us. We basically have to roll transactions > and redo all checks anyway. > > > > Hmmm, looking at the existing -EAGAIN case, I suspect this isn't > > > handled correctly by xfs_map_blocks() anymore. i.e. it just returns > > > the error which can lead to discarding the page rather than checking > > > to see if the there was a valid map allocated. I think there's some > > > followup work here (another patch series). :/ > > > > > > > Ok. At the moment, that error looks like it should only happen if we're > > past EOF..? Either way, the XFS_BMAPI_DELALLOC thing still can result in > > an error so it probably makes sense to tie a seqno check to -EAGAIN and > > handle it properly in the caller. > > For that whole -EAGAIN handling please look at my always cow series > again, I got bitten by it a few times and also think the current code > works only by chance and in the right phase of the moon. I hope the > series documents what we had it for very nicely. 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: 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. 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. 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. * * If the external change did not modify the current * mapping (or just grew it) this will have no effect. * If the current mapping shrunk, we expect to at * minimum still have blocks backing the current page as * the page has remained locked since writeback first * located delalloc block(s) at the page offset. A * racing truncate, hole punch or even reflink must wait * on page writeback before it can modify our page and * underlying block(s). * * We'll update *seq before we drop ilock for the next * iteration. */ if (*seq != READ_ONCE(ifp->if_seq)) { if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &icur, &timap) || timap.br_startoff > offset_fsb) { ASSERT(0); error = -EFSCORRUPTED; goto trans_cancel; } xfs_trim_extent(imap, timap.br_startoff, timap.br_blockcount); count_fsb = imap->br_blockcount; map_start_fsb = imap->br_startoff; } ... and getting rid of the existing i_size cruft. I think this handles the same problem in a different way, primary difference being that truncate or hole punch is more likely to have to wait on writeback rather than writeback trying so hard to get out of the way. Also note that we still have the i_size checks on the page in xfs_do_writepage() that will cause writeback to back off in the truncate case once we spin around to the next page. Thoughts? I'm still testing this but I can try to get something posted to the list a bit sooner than I was anticipating for the purpose of trying to order these series and/or sanity checking the approach.. Brian
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.
On Thu, Jan 17, 2019 at 08:41:48AM -0800, Christoph Hellwig wrote: > 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. > Ok, then I probably need to take a closer look. The purpose of these patches are to remove it and replace it with something that fundamentally addresses the underlying problem (i.e., the fork change detection). > > > > 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. > Yes, that's really just semantics. The purpose of the lookup in this context is to trim down the range to map. We can only guarantee the range specified by the current page once we cycle ilock, so we have to consider that any part of the range external to that has become invalid. This change to xfs_iomap_write_allocate() doesn't introduce any new way of using the caller's imap that isn't already done by the existing code. We just access the inode fork to validate the range rather than the inode size because the caller already gives us information to confirm whether the range has been invalidated (the *seq param) whereas the i_size could have been truncated down and up since the last time we checked it. > 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. I'm assuming that a correct solution will lend itself to cleaning up much of this code to do things like reduce the need for validations, provide commonality with other paths, clean up layering, etc., but I'm not worrying about that until we're confident that this is a correct and viable approach. Brian
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index d9048bcea49c..33a1be5df99f 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -29,6 +29,7 @@ struct xfs_writepage_ctx { struct xfs_bmbt_irec imap; unsigned int io_type; + unsigned int data_seq; unsigned int cow_seq; struct xfs_ioend *ioend; }; @@ -347,7 +348,8 @@ xfs_map_blocks( * out that ensures that we always see the current value. */ imap_valid = offset_fsb >= wpc->imap.br_startoff && - offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount; + offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount && + wpc->data_seq == READ_ONCE(ip->i_df.if_seq); if (imap_valid && (!xfs_inode_has_cow_data(ip) || wpc->io_type == XFS_IO_COW || @@ -417,6 +419,7 @@ xfs_map_blocks( */ if (!xfs_iext_lookup_extent(ip, &ip->i_df, offset_fsb, &icur, &imap)) imap.br_startoff = end_fsb; /* fake a hole past EOF */ + wpc->data_seq = READ_ONCE(ip->i_df.if_seq); xfs_iunlock(ip, XFS_ILOCK_SHARED); if (imap.br_startoff > offset_fsb) { @@ -454,7 +457,8 @@ xfs_map_blocks( return 0; allocate_blocks: error = xfs_iomap_write_allocate(ip, whichfork, offset, &imap, - &wpc->cow_seq); + whichfork == XFS_COW_FORK ? + &wpc->cow_seq : &wpc->data_seq); if (error) return error; ASSERT(whichfork == XFS_COW_FORK || cow_fsb == NULLFILEOFF || diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c index 27c93b5f029d..0401e33d4e8f 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -681,7 +681,7 @@ xfs_iomap_write_allocate( int whichfork, xfs_off_t offset, xfs_bmbt_irec_t *imap, - unsigned int *cow_seq) + unsigned int *seq) { xfs_mount_t *mp = ip->i_mount; struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); @@ -798,7 +798,7 @@ xfs_iomap_write_allocate( goto error0; if (whichfork == XFS_COW_FORK) - *cow_seq = READ_ONCE(ifp->if_seq); + *seq = READ_ONCE(ifp->if_seq); xfs_iunlock(ip, XFS_ILOCK_EXCL); }
The writeback code caches the current extent mapping across multiple xfs_do_writepage() calls to avoid repeated lookups for sequential pages backed by the same extent. This is known to be slightly racy with extent fork changes in certain difficult to reproduce scenarios. The cached extent is trimmed to within EOF to help avoid the most common vector for this problem via speculative preallocation management, but this is a band-aid that does not address the fundamental problem. Now that we have an xfs_ifork sequence counter mechanism used to facilitate COW writeback, we can use the same mechanism to validate consistency between the data fork and cached writeback mappings. On its face, this is somewhat of a big hammer approach because any change to the data fork invalidates any mapping currently cached by a writeback in progress regardless of whether the data fork change overlaps with the range under writeback. In practice, however, the impact of this approach is minimal in most cases. First, data fork changes (delayed allocations) caused by sustained sequential buffered writes are amortized across speculative preallocations. This means that a cached mapping won't be invalidated by each buffered write of a common file copy workload, but rather only on less frequent allocation events. Second, the extent tree is always entirely in-core so an additional lookup of a usable extent mostly costs a shared ilock cycle and in-memory tree lookup. This means that a cached mapping reval is relatively cheap compared to the I/O itself. Third, spurious invalidations don't impact ioend construction. This means that even if the same extent is revalidated multiple times across multiple writepage instances, we still construct and submit the same size ioend (and bio) if the blocks are physically contiguous. Update struct xfs_writepage_ctx with a new field to hold the sequence number of the data fork associated with the currently cached mapping. Check the wpc seqno against the data fork when the mapping is validated and reestablish the mapping whenever the fork has changed since the mapping was cached. This ensures that writeback always uses a valid extent mapping and thus prevents lost writebacks and stale delalloc block problems. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_aops.c | 8 ++++++-- fs/xfs/xfs_iomap.c | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-)