| Message ID | 20170403121833.7825-3-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Apr 03, 2017 at 02:18:29PM +0200, Christoph Hellwig wrote: > We never do COW operations for the attr fork, so don't pretend we handle > them. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_bmap.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 2a426d127e05..47b909c27bb3 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -6491,7 +6491,6 @@ xfs_bmap_finish_one( > struct xfs_bmbt_irec bmap; > int nimaps = 1; > xfs_fsblock_t firstfsb; > - int flags = XFS_BMAPI_REMAP; > int done; > int error = 0; > > @@ -6505,10 +6504,8 @@ xfs_bmap_finish_one( > XFS_FSB_TO_AGBNO(tp->t_mountp, startblock), > ip->i_ino, whichfork, startoff, blockcount, state); > > - if (whichfork != XFS_DATA_FORK && whichfork != XFS_ATTR_FORK) > + if (whichfork != XFS_DATA_FORK) > return -EFSCORRUPTED; > - if (whichfork == XFS_ATTR_FORK) > - flags |= XFS_BMAPI_ATTRFORK; Hmmmm. We never schedule deferred bmap operations for the attr fork, but all that stuff got plumbed all the way through to the bmap log item that is written to disk. Therefore, it's possible that we may some day replay a log from a future XFS with a deferred attr fork bmap item in the log, at which point log recovery blows up here. I wonder if this might be worth a WARN_ON_ONCE just to make it obvious that recovery stopped because it doesn't handle deferred attr fork bmap updates? --D > if (XFS_TEST_ERROR(false, tp->t_mountp, > XFS_ERRTAG_BMAP_FINISH_ONE, > @@ -6519,13 +6516,13 @@ xfs_bmap_finish_one( > case XFS_BMAP_MAP: > firstfsb = bmap.br_startblock; > error = xfs_bmapi_write(tp, ip, bmap.br_startoff, > - bmap.br_blockcount, flags, &firstfsb, > + bmap.br_blockcount, XFS_BMAPI_REMAP, &firstfsb, > bmap.br_blockcount, &bmap, &nimaps, > dfops); > break; > case XFS_BMAP_UNMAP: > error = xfs_bunmapi(tp, ip, bmap.br_startoff, > - bmap.br_blockcount, flags, 1, &firstfsb, > + bmap.br_blockcount, XFS_BMAPI_REMAP, 1, &firstfsb, > dfops, &done); > ASSERT(done); > break; > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 10, 2017 at 10:05:39AM -0700, Darrick J. Wong wrote: > Hmmmm. We never schedule deferred bmap operations for the attr fork, > but all that stuff got plumbed all the way through to the bmap log item > that is written to disk. Therefore, it's possible that we may some day > replay a log from a future XFS with a deferred attr fork bmap item in > the log, at which point log recovery blows up here. If we add that it's a format change that needs a incompat feature flag. > I wonder if this might be worth a WARN_ON_ONCE just to make it obvious > that recovery stopped because it doesn't handle deferred attr fork bmap > updates? Sure, although we should never actually hit it. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 2a426d127e05..47b909c27bb3 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -6491,7 +6491,6 @@ xfs_bmap_finish_one( struct xfs_bmbt_irec bmap; int nimaps = 1; xfs_fsblock_t firstfsb; - int flags = XFS_BMAPI_REMAP; int done; int error = 0; @@ -6505,10 +6504,8 @@ xfs_bmap_finish_one( XFS_FSB_TO_AGBNO(tp->t_mountp, startblock), ip->i_ino, whichfork, startoff, blockcount, state); - if (whichfork != XFS_DATA_FORK && whichfork != XFS_ATTR_FORK) + if (whichfork != XFS_DATA_FORK) return -EFSCORRUPTED; - if (whichfork == XFS_ATTR_FORK) - flags |= XFS_BMAPI_ATTRFORK; if (XFS_TEST_ERROR(false, tp->t_mountp, XFS_ERRTAG_BMAP_FINISH_ONE, @@ -6519,13 +6516,13 @@ xfs_bmap_finish_one( case XFS_BMAP_MAP: firstfsb = bmap.br_startblock; error = xfs_bmapi_write(tp, ip, bmap.br_startoff, - bmap.br_blockcount, flags, &firstfsb, + bmap.br_blockcount, XFS_BMAPI_REMAP, &firstfsb, bmap.br_blockcount, &bmap, &nimaps, dfops); break; case XFS_BMAP_UNMAP: error = xfs_bunmapi(tp, ip, bmap.br_startoff, - bmap.br_blockcount, flags, 1, &firstfsb, + bmap.br_blockcount, XFS_BMAPI_REMAP, 1, &firstfsb, dfops, &done); ASSERT(done); break;
We never do COW operations for the attr fork, so don't pretend we handle them. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_bmap.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)