* [PATCH v2 0/3] xfs: fix sf to block inode fork logging
@ 2019-10-07 13:19 Brian Foster
2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Brian Foster @ 2019-10-07 13:19 UTC (permalink / raw)
To: linux-xfs
Hi all,
Here's v2 of the directory inode shortform to block logging fixup. This
also addresses the similar attr fork conversion function. I put the
other cleanups into separate patches because it wasn't totally clear to
me if we wanted to add the log call to the conversion function given the
other callers log the inode outside of that function. IOW, we can either
keep or drop patch 3. I have no strong preference either way.
Brian
v2:
- Also fix up attr fork conversion.
- Add patches 2 and 3 for follow up cleanups.
v1: https://lore.kernel.org/linux-xfs/20191004125520.7857-1-bfoster@redhat.com/
Brian Foster (3):
xfs: log the inode on directory sf to block format change
xfs: remove broken error handling on failed attr sf to leaf change
xfs: move local to extent inode logging into bmap helper
fs/xfs/libxfs/xfs_attr_leaf.c | 21 +++------------------
fs/xfs/libxfs/xfs_bmap.c | 6 ++++--
fs/xfs/libxfs/xfs_bmap.h | 3 ++-
fs/xfs/libxfs/xfs_dir2_block.c | 2 +-
4 files changed, 10 insertions(+), 22 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 1/3] xfs: log the inode on directory sf to block format change 2019-10-07 13:19 [PATCH v2 0/3] xfs: fix sf to block inode fork logging Brian Foster @ 2019-10-07 13:19 ` Brian Foster 2019-10-08 6:59 ` Christoph Hellwig 2019-10-08 16:11 ` Darrick J. Wong 2019-10-07 13:19 ` [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change Brian Foster 2019-10-07 13:19 ` [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper Brian Foster 2 siblings, 2 replies; 10+ messages in thread From: Brian Foster @ 2019-10-07 13:19 UTC (permalink / raw) To: linux-xfs 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(+) 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] xfs: log the inode on directory sf to block format change 2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster @ 2019-10-08 6:59 ` Christoph Hellwig 2019-10-08 16:11 ` Darrick J. Wong 1 sibling, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2019-10-08 6:59 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] xfs: log the inode on directory sf to block format change 2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster 2019-10-08 6:59 ` Christoph Hellwig @ 2019-10-08 16:11 ` Darrick J. Wong 1 sibling, 0 replies; 10+ messages in thread From: Darrick J. Wong @ 2019-10-08 16:11 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change 2019-10-07 13:19 [PATCH v2 0/3] xfs: fix sf to block inode fork logging Brian Foster 2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster @ 2019-10-07 13:19 ` Brian Foster 2019-10-08 7:00 ` Christoph Hellwig 2019-10-08 16:11 ` Darrick J. Wong 2019-10-07 13:19 ` [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper Brian Foster 2 siblings, 2 replies; 10+ messages in thread From: Brian Foster @ 2019-10-07 13:19 UTC (permalink / raw) To: linux-xfs xfs_attr_shortform_to_leaf() attempts to put the shortform fork back together after a failed attempt to convert from shortform to leaf format. While this code reallocates and copies back the shortform attr fork data, it never resets the inode format field back to local format. Further, now that the inode is properly logged after the initial switch from local format, any error that triggers the recovery code will eventually abort the transaction and shutdown the fs. Therefore, remove the broken and unnecessary error handling code. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/libxfs/xfs_attr_leaf.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index 36c0a32cefcf..1b956c313b6b 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -831,28 +831,13 @@ xfs_attr_shortform_to_leaf( bp = NULL; error = xfs_da_grow_inode(args, &blkno); - if (error) { - /* - * If we hit an IO error middle of the transaction inside - * grow_inode(), we may have inconsistent data. Bail out. - */ - if (error == -EIO) - goto out; - xfs_idata_realloc(dp, size, XFS_ATTR_FORK); /* try to put */ - memcpy(ifp->if_u1.if_data, tmpbuffer, size); /* it back */ + if (error) goto out; - } ASSERT(blkno == 0); error = xfs_attr3_leaf_create(args, blkno, &bp); - if (error) { - /* xfs_attr3_leaf_create may not have instantiated a block */ - if (bp && (xfs_da_shrink_inode(args, 0, bp) != 0)) - goto out; - xfs_idata_realloc(dp, size, XFS_ATTR_FORK); /* try to put */ - memcpy(ifp->if_u1.if_data, tmpbuffer, size); /* it back */ + if (error) goto out; - } memset((char *)&nargs, 0, sizeof(nargs)); nargs.dp = dp; -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change 2019-10-07 13:19 ` [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change Brian Foster @ 2019-10-08 7:00 ` Christoph Hellwig 2019-10-08 16:11 ` Darrick J. Wong 1 sibling, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2019-10-08 7:00 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs On Mon, Oct 07, 2019 at 09:19:37AM -0400, Brian Foster wrote: > xfs_attr_shortform_to_leaf() attempts to put the shortform fork back > together after a failed attempt to convert from shortform to leaf > format. While this code reallocates and copies back the shortform > attr fork data, it never resets the inode format field back to local > format. Further, now that the inode is properly logged after the > initial switch from local format, any error that triggers the > recovery code will eventually abort the transaction and shutdown the > fs. Therefore, remove the broken and unnecessary error handling > code. > > Signed-off-by: Brian Foster <bfoster@redhat.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change 2019-10-07 13:19 ` [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change Brian Foster 2019-10-08 7:00 ` Christoph Hellwig @ 2019-10-08 16:11 ` Darrick J. Wong 1 sibling, 0 replies; 10+ messages in thread From: Darrick J. Wong @ 2019-10-08 16:11 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs On Mon, Oct 07, 2019 at 09:19:37AM -0400, Brian Foster wrote: > xfs_attr_shortform_to_leaf() attempts to put the shortform fork back > together after a failed attempt to convert from shortform to leaf > format. While this code reallocates and copies back the shortform > attr fork data, it never resets the inode format field back to local > format. Further, now that the inode is properly logged after the > initial switch from local format, any error that triggers the > recovery code will eventually abort the transaction and shutdown the > fs. Therefore, remove the broken and unnecessary error handling > code. > > 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 | 19 ++----------------- > 1 file changed, 2 insertions(+), 17 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index 36c0a32cefcf..1b956c313b6b 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -831,28 +831,13 @@ xfs_attr_shortform_to_leaf( > > bp = NULL; > error = xfs_da_grow_inode(args, &blkno); > - if (error) { > - /* > - * If we hit an IO error middle of the transaction inside > - * grow_inode(), we may have inconsistent data. Bail out. > - */ > - if (error == -EIO) > - goto out; > - xfs_idata_realloc(dp, size, XFS_ATTR_FORK); /* try to put */ > - memcpy(ifp->if_u1.if_data, tmpbuffer, size); /* it back */ > + if (error) > goto out; > - } > > ASSERT(blkno == 0); > error = xfs_attr3_leaf_create(args, blkno, &bp); > - if (error) { > - /* xfs_attr3_leaf_create may not have instantiated a block */ > - if (bp && (xfs_da_shrink_inode(args, 0, bp) != 0)) > - goto out; > - xfs_idata_realloc(dp, size, XFS_ATTR_FORK); /* try to put */ > - memcpy(ifp->if_u1.if_data, tmpbuffer, size); /* it back */ > + if (error) > goto out; > - } > > memset((char *)&nargs, 0, sizeof(nargs)); > nargs.dp = dp; > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper 2019-10-07 13:19 [PATCH v2 0/3] xfs: fix sf to block inode fork logging Brian Foster 2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster 2019-10-07 13:19 ` [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change Brian Foster @ 2019-10-07 13:19 ` Brian Foster 2019-10-08 7:00 ` Christoph Hellwig 2019-10-08 16:11 ` Darrick J. Wong 2 siblings, 2 replies; 10+ messages in thread From: Brian Foster @ 2019-10-07 13:19 UTC (permalink / raw) To: linux-xfs The callers of xfs_bmap_local_to_extents_empty() log the inode external to the function, yet this function is where the on-disk format value is updated. Push the inode logging down into the function itself to help prevent future mistakes. Note that internal bmap callers track the inode logging flags independently and thus may log the inode core twice due to this change. This is harmless, so leave this code around for consistency with the other attr fork conversion functions. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/libxfs/xfs_attr_leaf.c | 3 +-- fs/xfs/libxfs/xfs_bmap.c | 6 ++++-- fs/xfs/libxfs/xfs_bmap.h | 3 ++- fs/xfs/libxfs/xfs_dir2_block.c | 3 +-- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index 1b956c313b6b..f0089e862216 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -826,8 +826,7 @@ xfs_attr_shortform_to_leaf( sf = (xfs_attr_shortform_t *)tmpbuffer; 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); + xfs_bmap_local_to_extents_empty(args->trans, dp, XFS_ATTR_FORK); bp = NULL; error = xfs_da_grow_inode(args, &blkno); diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 4edc25a2ba80..02469d59c787 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -792,6 +792,7 @@ xfs_bmap_extents_to_btree( */ void xfs_bmap_local_to_extents_empty( + struct xfs_trans *tp, struct xfs_inode *ip, int whichfork) { @@ -808,6 +809,7 @@ xfs_bmap_local_to_extents_empty( ifp->if_u1.if_root = NULL; ifp->if_height = 0; XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS); + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); } @@ -840,7 +842,7 @@ xfs_bmap_local_to_extents( ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL); if (!ifp->if_bytes) { - xfs_bmap_local_to_extents_empty(ip, whichfork); + xfs_bmap_local_to_extents_empty(tp, ip, whichfork); flags = XFS_ILOG_CORE; goto done; } @@ -887,7 +889,7 @@ xfs_bmap_local_to_extents( /* account for the change in fork size */ xfs_idata_realloc(ip, -ifp->if_bytes, whichfork); - xfs_bmap_local_to_extents_empty(ip, whichfork); + xfs_bmap_local_to_extents_empty(tp, ip, whichfork); flags |= XFS_ILOG_CORE; ifp->if_u1.if_root = NULL; diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h index 5bb446d80542..e2798c6f3a5f 100644 --- a/fs/xfs/libxfs/xfs_bmap.h +++ b/fs/xfs/libxfs/xfs_bmap.h @@ -182,7 +182,8 @@ void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, xfs_filblks_t len); int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); -void xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork); +void xfs_bmap_local_to_extents_empty(struct xfs_trans *tp, + struct xfs_inode *ip, int whichfork); void __xfs_bmap_add_free(struct xfs_trans *tp, xfs_fsblock_t bno, xfs_filblks_t len, const struct xfs_owner_info *oinfo, bool skip_discard); diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c index 3d1e5f6d64fd..49e4bc39e7bb 100644 --- a/fs/xfs/libxfs/xfs_dir2_block.c +++ b/fs/xfs/libxfs/xfs_dir2_block.c @@ -1096,9 +1096,8 @@ xfs_dir2_sf_to_block( memcpy(sfp, oldsfp, ifp->if_bytes); xfs_idata_realloc(dp, -ifp->if_bytes, XFS_DATA_FORK); - xfs_bmap_local_to_extents_empty(dp, XFS_DATA_FORK); + xfs_bmap_local_to_extents_empty(tp, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper 2019-10-07 13:19 ` [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper Brian Foster @ 2019-10-08 7:00 ` Christoph Hellwig 2019-10-08 16:11 ` Darrick J. Wong 1 sibling, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2019-10-08 7:00 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs On Mon, Oct 07, 2019 at 09:19:38AM -0400, Brian Foster wrote: > The callers of xfs_bmap_local_to_extents_empty() log the inode > external to the function, yet this function is where the on-disk > format value is updated. Push the inode logging down into the > function itself to help prevent future mistakes. > > Note that internal bmap callers track the inode logging flags > independently and thus may log the inode core twice due to this > change. This is harmless, so leave this code around for consistency > with the other attr fork conversion functions. > > Signed-off-by: Brian Foster <bfoster@redhat.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper 2019-10-07 13:19 ` [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper Brian Foster 2019-10-08 7:00 ` Christoph Hellwig @ 2019-10-08 16:11 ` Darrick J. Wong 1 sibling, 0 replies; 10+ messages in thread From: Darrick J. Wong @ 2019-10-08 16:11 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs On Mon, Oct 07, 2019 at 09:19:38AM -0400, Brian Foster wrote: > The callers of xfs_bmap_local_to_extents_empty() log the inode > external to the function, yet this function is where the on-disk > format value is updated. Push the inode logging down into the > function itself to help prevent future mistakes. > > Note that internal bmap callers track the inode logging flags > independently and thus may log the inode core twice due to this > change. This is harmless, so leave this code around for consistency > with the other attr fork conversion functions. > > 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 | 3 +-- > fs/xfs/libxfs/xfs_bmap.c | 6 ++++-- > fs/xfs/libxfs/xfs_bmap.h | 3 ++- > fs/xfs/libxfs/xfs_dir2_block.c | 3 +-- > 4 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index 1b956c313b6b..f0089e862216 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -826,8 +826,7 @@ xfs_attr_shortform_to_leaf( > sf = (xfs_attr_shortform_t *)tmpbuffer; > > 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); > + xfs_bmap_local_to_extents_empty(args->trans, dp, XFS_ATTR_FORK); > > bp = NULL; > error = xfs_da_grow_inode(args, &blkno); > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 4edc25a2ba80..02469d59c787 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -792,6 +792,7 @@ xfs_bmap_extents_to_btree( > */ > void > xfs_bmap_local_to_extents_empty( > + struct xfs_trans *tp, > struct xfs_inode *ip, > int whichfork) > { > @@ -808,6 +809,7 @@ xfs_bmap_local_to_extents_empty( > ifp->if_u1.if_root = NULL; > ifp->if_height = 0; > XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS); > + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); > } > > > @@ -840,7 +842,7 @@ xfs_bmap_local_to_extents( > ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL); > > if (!ifp->if_bytes) { > - xfs_bmap_local_to_extents_empty(ip, whichfork); > + xfs_bmap_local_to_extents_empty(tp, ip, whichfork); > flags = XFS_ILOG_CORE; > goto done; > } > @@ -887,7 +889,7 @@ xfs_bmap_local_to_extents( > > /* account for the change in fork size */ > xfs_idata_realloc(ip, -ifp->if_bytes, whichfork); > - xfs_bmap_local_to_extents_empty(ip, whichfork); > + xfs_bmap_local_to_extents_empty(tp, ip, whichfork); > flags |= XFS_ILOG_CORE; > > ifp->if_u1.if_root = NULL; > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h > index 5bb446d80542..e2798c6f3a5f 100644 > --- a/fs/xfs/libxfs/xfs_bmap.h > +++ b/fs/xfs/libxfs/xfs_bmap.h > @@ -182,7 +182,8 @@ void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, > xfs_filblks_t len); > int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); > int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); > -void xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork); > +void xfs_bmap_local_to_extents_empty(struct xfs_trans *tp, > + struct xfs_inode *ip, int whichfork); > void __xfs_bmap_add_free(struct xfs_trans *tp, xfs_fsblock_t bno, > xfs_filblks_t len, const struct xfs_owner_info *oinfo, > bool skip_discard); > diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c > index 3d1e5f6d64fd..49e4bc39e7bb 100644 > --- a/fs/xfs/libxfs/xfs_dir2_block.c > +++ b/fs/xfs/libxfs/xfs_dir2_block.c > @@ -1096,9 +1096,8 @@ xfs_dir2_sf_to_block( > memcpy(sfp, oldsfp, ifp->if_bytes); > > xfs_idata_realloc(dp, -ifp->if_bytes, XFS_DATA_FORK); > - xfs_bmap_local_to_extents_empty(dp, XFS_DATA_FORK); > + xfs_bmap_local_to_extents_empty(tp, 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-08 16:12 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-07 13:19 [PATCH v2 0/3] xfs: fix sf to block inode fork logging Brian Foster 2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster 2019-10-08 6:59 ` Christoph Hellwig 2019-10-08 16:11 ` Darrick J. Wong 2019-10-07 13:19 ` [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change Brian Foster 2019-10-08 7:00 ` Christoph Hellwig 2019-10-08 16:11 ` Darrick J. Wong 2019-10-07 13:19 ` [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper Brian Foster 2019-10-08 7:00 ` Christoph Hellwig 2019-10-08 16:11 ` Darrick J. Wong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).