On 2/26/20 3:34 PM, Dave Chinner wrote:
On Tue, Feb 25, 2020 at 04:57:46PM -0800, Allison Collins wrote:
On 2/25/20 1:57 AM, Dave Chinner wrote:
On Sat, Feb 22, 2020 at 07:06:05PM -0700, Allison Collins wrote:
+out:
+ return error;
+}
Brian commented on the structure of this loop better than I could.
+
+/*
+ * Remove the attribute specified in @args.
+ *
+ * This function may return -EAGAIN to signal that the transaction needs to be
+ * rolled. Callers should continue calling this function until they receive a
+ * return value other than -EAGAIN.
+ */
+int
+xfs_attr_remove_iter(
struct xfs_da_args *args)
{
struct xfs_inode *dp = args->dp;
int error;
+ /* State machine switch */
+ switch (args->dac.dela_state) {
+ case XFS_DAS_RM_SHRINK:
+ case XFS_DAS_RMTVAL_REMOVE:
+ goto node;
+ default:
+ break;
+ }
Why separate out the state machine? Doesn't this shortcut the
xfs_inode_hasattr() check? Shouldn't that come first?
Well, the idea is that when we first start the routine, we come in with
neither state set, and we fall through to the break. So we execute the
check the first time through.
Though now that you point it out, I should probably go back and put the
explicit numbering back in the enum (starting with 1) or they will default
to zero, which would be incorrect. I had pulled it out in one of the last
reviews thinking it would be ok, but it should go back in.
As it is:
case XFS_DAS_RM_SHRINK:
case XFS_DAS_RMTVAL_REMOVE:
return xfs_attr_node_removename(args);
default:
break;
would be nicer, and if this is the only way we can get to
xfs_attr_node_removename(c, getting rid of it from the code
below could be done, too.
Well, the remove path is a lot simpler than the set path, so that trick does
work here :-)
The idea though was to establish "jump points" with the "XFS_DAS_*" states.
Based on the state, we jump back to where we were. We could break this
pattern for the remove path, but I dont think we'd want to do the same for
the others. The set routine is a really big function that would end up
being inside a really big switch!
Right, which is why I think it should be factored into function
calls first, then the switch statement simply becomes a small set of
function calls.
We use that pattern quite a bit in the da_btree code to call
the correct dir/attr function based on the type of block we are
manipulating (i.e. based on da_state context). e.g. xfs_da3_split(),
xfs_da3_join(), etc.
I see, sure will do. The patches were ordered much that way in the last
version, so it wouldnt be hard to undo.
struct xfs_da_geometry *geo; /* da block geometry */
struct xfs_name name; /* name, length and argument flags*/
uint8_t filetype; /* filetype of inode for directories */
diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index 1887605..9a649d1 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -24,6 +24,8 @@
#include "xfs_rmap_btree.h"
#include "xfs_log.h"
#include "xfs_trans_priv.h"
+#include "xfs_da_format.h"
+#include "xfs_da_btree.h"
#include "xfs_attr.h"
#include "xfs_reflink.h"
#include "scrub/scrub.h"
Hmmm - why are these new includes necessary? You didn't add anything
new to these files or common header files to make the includes
needed....
Because the delayed attr context uses things from those headers. And we put
the context in xfs_da_args. Now everything that uses xfs_da_args needs
those includes. But maybe if we do what you suggest above, we wont need to.
:-)
put:
struct xfs_da_state;
and whatever other forward declarations are require for the pointer
types used in the delayed attr context at the top of xfs_attr.h.
These are just pointers in the structure, so we don't need the full
structure definitions if the pointers aren't actually dereferenced
by the code that includes the header file.
Alrighty, will fix.
Thanks for the reviews!
Allison
Cheers,
Dave.
