| Message ID | 170120319438.13206.6231336717299702762.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: log intent item recovery should reconstruct defer work state | expand |
On Tue, Nov 28, 2023 at 12:26:34PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > If recovery finds an xattr log intent item calling for the removal of an > attribute and the file doesn't even have an attr fork, we know that the > removal is trivially complete. However, we can't just exit the recovery > function without doing something about the recovered log intent item -- > it's still on the AIL, and not logging an attrd item means it stays > there forever. > > This has likely not been seen in practice because few people use LARP > and the runtime code won't log the attri for a no-attrfork removexattr > operation. But let's fix this anyway. > > Also we shouldn't really be testing the attr fork presence until we've > taken the ILOCK, though this doesn't matter much in recovery, which is > single threaded. > > Fixes: fdaf1bb3cafc ("xfs: ATTR_REPLACE algorithm with LARP enabled needs rework") No useful comment here as the attr logging code is new to me, but what is the LARP mode? I see plenty of references to it in commit logs, a small amount in the code mostly related to error injection, but it would be really good to expand the acronym somehwere as I can't find any explanation in the code or commit logs..
On Wed, Nov 29, 2023 at 11:34:41PM -0800, Christoph Hellwig wrote: > On Tue, Nov 28, 2023 at 12:26:34PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > If recovery finds an xattr log intent item calling for the removal of an > > attribute and the file doesn't even have an attr fork, we know that the > > removal is trivially complete. However, we can't just exit the recovery > > function without doing something about the recovered log intent item -- > > it's still on the AIL, and not logging an attrd item means it stays > > there forever. > > > > This has likely not been seen in practice because few people use LARP > > and the runtime code won't log the attri for a no-attrfork removexattr > > operation. But let's fix this anyway. > > > > Also we shouldn't really be testing the attr fork presence until we've > > taken the ILOCK, though this doesn't matter much in recovery, which is > > single threaded. > > > > Fixes: fdaf1bb3cafc ("xfs: ATTR_REPLACE algorithm with LARP enabled needs rework") > > No useful comment here as the attr logging code is new to me, but what > is the LARP mode? I see plenty of references to it in commit logs, > a small amount in the code mostly related to error injection, but it > would be really good to expand the acronym somehwere as I can't find > any explanation in the code or commit logs.. LARP == Logged extended Attributes via Replay Persistence (IOWs, a silly developer acronym for writing attr log intent items.) --D
On Thu, Nov 30, 2023 at 09:02:36AM -0800, Darrick J. Wong wrote: > > No useful comment here as the attr logging code is new to me, but what > > is the LARP mode? I see plenty of references to it in commit logs, > > a small amount in the code mostly related to error injection, but it > > would be really good to expand the acronym somehwere as I can't find > > any explanation in the code or commit logs.. > > LARP == Logged extended Attributes via Replay Persistence > > (IOWs, a silly developer acronym for writing attr log intent items.) We should probably spell it out somewhere. I also found comments it's a debug only feature, which also confuses me.
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c index 36fe2abb16e6..11e88a76a33c 100644 --- a/fs/xfs/xfs_attr_item.c +++ b/fs/xfs/xfs_attr_item.c @@ -329,6 +329,13 @@ xfs_xattri_finish_update( goto out; } + /* If an attr removal is trivially complete, we're done. */ + if (attr->xattri_op_flags == XFS_ATTRI_OP_FLAGS_REMOVE && + !xfs_inode_hasattr(args->dp)) { + error = 0; + goto out; + } + error = xfs_attr_set_iter(attr); if (!error && attr->xattri_dela_state != XFS_DAS_DONE) error = -EAGAIN; @@ -608,8 +615,6 @@ xfs_attri_item_recover( attr->xattri_dela_state = xfs_attr_init_add_state(args); break; case XFS_ATTRI_OP_FLAGS_REMOVE: - if (!xfs_inode_hasattr(args->dp)) - goto out; attr->xattri_dela_state = xfs_attr_init_remove_state(args); break; default: