| Message ID | 20191007131938.23839-2-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: fix sf to block inode fork logging | expand |
On Mon, Oct 07, 2019 at 09:19:36AM -0400, Brian Foster wrote: > When a directory changes from shortform (sf) to block format, the sf > format is copied to a temporary buffer, the inode format is modified > and the updated format filled with the dentries from the temporary > buffer. If the inode format is modified and attempt to grow the > inode fails (due to I/O error, for example), it is possible to > return an error while leaving the directory in an inconsistent state > and with an otherwise clean transaction. This results in corruption > of the associated directory and leads to xfs_dabuf_map() errors as > subsequent lookups cannot accurately determine the format of the > directory. This problem is reproduced occasionally by generic/475. > > The fundamental problem is that xfs_dir2_sf_to_block() changes the > on-disk inode format without logging the inode. The inode is > eventually logged by the bmapi layer in the common case, but error > checking introduces the possibility of failing the high level > request before this happens. > > Update both of the dir2 and attr callers of > xfs_bmap_local_to_extents_empty() to log the inode core as > consistent with the bmap local to extent format change codepath. > This ensures that any subsequent errors after the format has changed > cause the transaction to abort. > > Signed-off-by: Brian Foster <bfoster@redhat.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
On Mon, Oct 07, 2019 at 09:19:36AM -0400, Brian Foster wrote: > When a directory changes from shortform (sf) to block format, the sf > format is copied to a temporary buffer, the inode format is modified > and the updated format filled with the dentries from the temporary > buffer. If the inode format is modified and attempt to grow the > inode fails (due to I/O error, for example), it is possible to > return an error while leaving the directory in an inconsistent state > and with an otherwise clean transaction. This results in corruption > of the associated directory and leads to xfs_dabuf_map() errors as > subsequent lookups cannot accurately determine the format of the > directory. This problem is reproduced occasionally by generic/475. > > The fundamental problem is that xfs_dir2_sf_to_block() changes the > on-disk inode format without logging the inode. The inode is > eventually logged by the bmapi layer in the common case, but error > checking introduces the possibility of failing the high level > request before this happens. > > Update both of the dir2 and attr callers of > xfs_bmap_local_to_extents_empty() to log the inode core as > consistent with the bmap local to extent format change codepath. > This ensures that any subsequent errors after the format has changed > cause the transaction to abort. > > Signed-off-by: Brian Foster <bfoster@redhat.com> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/libxfs/xfs_attr_leaf.c | 1 + > fs/xfs/libxfs/xfs_dir2_block.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index b9f019603d0b..36c0a32cefcf 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -827,6 +827,7 @@ xfs_attr_shortform_to_leaf( > > xfs_idata_realloc(dp, -size, XFS_ATTR_FORK); > xfs_bmap_local_to_extents_empty(dp, XFS_ATTR_FORK); > + xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE); > > bp = NULL; > error = xfs_da_grow_inode(args, &blkno); > diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c > index 9595ced393dc..3d1e5f6d64fd 100644 > --- a/fs/xfs/libxfs/xfs_dir2_block.c > +++ b/fs/xfs/libxfs/xfs_dir2_block.c > @@ -1098,6 +1098,7 @@ xfs_dir2_sf_to_block( > xfs_idata_realloc(dp, -ifp->if_bytes, XFS_DATA_FORK); > xfs_bmap_local_to_extents_empty(dp, XFS_DATA_FORK); > dp->i_d.di_size = 0; > + xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE); > > /* > * Add block 0 to the inode. > -- > 2.20.1 >
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index b9f019603d0b..36c0a32cefcf 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -827,6 +827,7 @@ xfs_attr_shortform_to_leaf( xfs_idata_realloc(dp, -size, XFS_ATTR_FORK); xfs_bmap_local_to_extents_empty(dp, XFS_ATTR_FORK); + xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE); bp = NULL; error = xfs_da_grow_inode(args, &blkno); diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c index 9595ced393dc..3d1e5f6d64fd 100644 --- a/fs/xfs/libxfs/xfs_dir2_block.c +++ b/fs/xfs/libxfs/xfs_dir2_block.c @@ -1098,6 +1098,7 @@ xfs_dir2_sf_to_block( xfs_idata_realloc(dp, -ifp->if_bytes, XFS_DATA_FORK); xfs_bmap_local_to_extents_empty(dp, XFS_DATA_FORK); dp->i_d.di_size = 0; + xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE); /* * Add block 0 to the inode.
When a directory changes from shortform (sf) to block format, the sf format is copied to a temporary buffer, the inode format is modified and the updated format filled with the dentries from the temporary buffer. If the inode format is modified and attempt to grow the inode fails (due to I/O error, for example), it is possible to return an error while leaving the directory in an inconsistent state and with an otherwise clean transaction. This results in corruption of the associated directory and leads to xfs_dabuf_map() errors as subsequent lookups cannot accurately determine the format of the directory. This problem is reproduced occasionally by generic/475. The fundamental problem is that xfs_dir2_sf_to_block() changes the on-disk inode format without logging the inode. The inode is eventually logged by the bmapi layer in the common case, but error checking introduces the possibility of failing the high level request before this happens. Update both of the dir2 and attr callers of xfs_bmap_local_to_extents_empty() to log the inode core as consistent with the bmap local to extent format change codepath. This ensures that any subsequent errors after the format has changed cause the transaction to abort. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/libxfs/xfs_attr_leaf.c | 1 + fs/xfs/libxfs/xfs_dir2_block.c | 1 + 2 files changed, 2 insertions(+)