On Mon, Oct 21, 2024 at 8:14 PM David Howells <dhowells@xxxxxxxxxx> wrote:
>
> When rename moves an AFS subdirectory between parent directories, the
> subdir also needs a bit of editing: the ".." entry needs updating to point
> to the new parent (though I don't make use of the info) and the DV needs
> incrementing by 1 to reflect the change of content. The server also sends
> a callback break notification on the subdirectory if we have one, but we
> can take care of recovering the promise next time we access the subdir.
>
> This can be triggered by something like:
>
> mount -t afs %example.com:xfstest.test20 /xfstest.test/
> mkdir /xfstest.test/{aaa,bbbb,aaa/ccc}
> mv /xfstest.test/{aaa/ccc,bbbb/ccc}
> touch /xfstest.test/bbbb/ccc/d
>
> When the pathwalk for the touch hits "ccc", kafs spots that the DV is
> incorrect and downloads it again unnecessarily.
>
> Fix this, if the rename target is a directory and the old and new
> parents are different, by:
>
> (1) Incrementing the DV number of the target locally.
>
> (2) Editing the ".." entry in the target to refer to its new parent's
> vnode ID and uniquifier.
>
> cc: David Howells <dhowells@xxxxxxxxxx>
> cc: Marc Dionne <marc.dionne@xxxxxxxxxxxx>
> cc: linux-afs@xxxxxxxxxxxxxxxxxxx
> ---
> fs/afs/dir.c | 25 ++++++++++++
> fs/afs/dir_edit.c | 91 ++++++++++++++++++++++++++++++++++++++++++++-
> fs/afs/internal.h | 2
> include/trace/events/afs.h | 7 ++-
> 4 files changed, 122 insertions(+), 3 deletions(-)
>
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index f8622ed72e08..474062d22712 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -12,6 +12,7 @@
> #include <linux/swap.h>
> #include <linux/ctype.h>
> #include <linux/sched.h>
> +#include <linux/iversion.h>
> #include <linux/task_io_accounting_ops.h>
> #include "internal.h"
> #include "afs_fs.h"
> @@ -1823,6 +1824,8 @@ static int afs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>
> static void afs_rename_success(struct afs_operation *op)
> {
> + struct afs_vnode *vnode = AFS_FS_I(d_inode(op->dentry));
> +
> _enter("op=%08x", op->debug_id);
>
> op->ctime = op->file[0].scb.status.mtime_client;
> @@ -1832,6 +1835,22 @@ static void afs_rename_success(struct afs_operation *op)
> op->ctime = op->file[1].scb.status.mtime_client;
> afs_vnode_commit_status(op, &op->file[1]);
> }
> +
> + /* If we're moving a subdir between dirs, we need to update
> + * its DV counter too as the ".." will be altered.
> + */
> + if (S_ISDIR(vnode->netfs.inode.i_mode) &&
> + op->file[0].vnode != op->file[1].vnode) {
> + u64 new_dv;
> +
> + write_seqlock(&vnode->cb_lock);
> +
> + new_dv = vnode->status.data_version + 1;
> + vnode->status.data_version = new_dv;
> + inode_set_iversion_raw(&vnode->netfs.inode, new_dv);
> +
> + write_sequnlock(&vnode->cb_lock);
> + }
> }
>
> static void afs_rename_edit_dir(struct afs_operation *op)
> @@ -1873,6 +1892,12 @@ static void afs_rename_edit_dir(struct afs_operation *op)
> &vnode->fid, afs_edit_dir_for_rename_2);
> }
>
> + if (S_ISDIR(vnode->netfs.inode.i_mode) &&
> + new_dvnode != orig_dvnode &&
> + test_bit(AFS_VNODE_DIR_VALID, &vnode->flags))
> + afs_edit_dir_update_dotdot(vnode, new_dvnode,
> + afs_edit_dir_for_rename_sub);
> +
Empty line with some whitespace.
> new_inode = d_inode(new_dentry);
> if (new_inode) {
> spin_lock(&new_inode->i_lock);
> diff --git a/fs/afs/dir_edit.c b/fs/afs/dir_edit.c
> index a71bff10496b..fe223fb78111 100644
> --- a/fs/afs/dir_edit.c
> +++ b/fs/afs/dir_edit.c
> @@ -127,10 +127,10 @@ static struct folio *afs_dir_get_folio(struct afs_vnode *vnode, pgoff_t index)
> /*
> * Scan a directory block looking for a dirent of the right name.
> */
> -static int afs_dir_scan_block(union afs_xdr_dir_block *block, struct qstr *name,
> +static int afs_dir_scan_block(const union afs_xdr_dir_block *block, const struct qstr *name,
> unsigned int blocknum)
> {
> - union afs_xdr_dirent *de;
> + const union afs_xdr_dirent *de;
> u64 bitmap;
> int d, len, n;
>
> @@ -492,3 +492,90 @@ void afs_edit_dir_remove(struct afs_vnode *vnode,
> clear_bit(AFS_VNODE_DIR_VALID, &vnode->flags);
> goto out_unmap;
> }
> +
> +/*
> + * Edit a subdirectory that has been moved between directories to update the
> + * ".." entry.
> + */
> +void afs_edit_dir_update_dotdot(struct afs_vnode *vnode, struct afs_vnode *new_dvnode,
> + enum afs_edit_dir_reason why)
> +{
> + union afs_xdr_dir_block *block;
> + union afs_xdr_dirent *de;
> + struct folio *folio;
> + unsigned int nr_blocks, b;
> + pgoff_t index;
> + loff_t i_size;
> + int slot;
> +
> + _enter("");
> +
> + i_size = i_size_read(&vnode->netfs.inode);
> + if (i_size < AFS_DIR_BLOCK_SIZE) {
> + clear_bit(AFS_VNODE_DIR_VALID, &vnode->flags);
> + return;
> + }
> + nr_blocks = i_size / AFS_DIR_BLOCK_SIZE;
> +
> + /* Find a block that has sufficient slots available. Each folio
> + * contains two or more directory blocks.
> + */
> + for (b = 0; b < nr_blocks; b++) {
> + index = b / AFS_DIR_BLOCKS_PER_PAGE;
> + folio = afs_dir_get_folio(vnode, index);
> + if (!folio)
> + goto error;
> +
> + block = kmap_local_folio(folio, b * AFS_DIR_BLOCK_SIZE - folio_pos(folio));
> +
> + /* Abandon the edit if we got a callback break. */
> + if (!test_bit(AFS_VNODE_DIR_VALID, &vnode->flags))
> + goto invalidated;
> +
> + slot = afs_dir_scan_block(block, &dotdot_name, b);
> + if (slot >= 0)
> + goto found_dirent;
> +
> + kunmap_local(block);
> + folio_unlock(folio);
> + folio_put(folio);
> + }
> +
> + /* Didn't find the dirent to clobber. Download the directory again. */
> + trace_afs_edit_dir(vnode, why, afs_edit_dir_update_nodd,
> + 0, 0, 0, 0, "..");
> + clear_bit(AFS_VNODE_DIR_VALID, &vnode->flags);
> + goto out;
> +
> +found_dirent:
> + de = &block->dirents[slot];
> + de->u.vnode = htonl(new_dvnode->fid.vnode);
> + de->u.unique = htonl(new_dvnode->fid.unique);
> +
> + trace_afs_edit_dir(vnode, why, afs_edit_dir_update_dd, b, slot,
> + ntohl(de->u.vnode), ntohl(de->u.unique), "..");
> +
> + kunmap_local(block);
> + folio_unlock(folio);
> + folio_put(folio);
> + inode_set_iversion_raw(&vnode->netfs.inode, vnode->status.data_version);
> +
> +out:
> + _leave("");
> + return;
> +
> +invalidated:
> + kunmap_local(block);
> + folio_unlock(folio);
> + folio_put(folio);
> + trace_afs_edit_dir(vnode, why, afs_edit_dir_update_inval,
> + 0, 0, 0, 0, "..");
> + clear_bit(AFS_VNODE_DIR_VALID, &vnode->flags);
> + goto out;
> +
> +error:
> + trace_afs_edit_dir(vnode, why, afs_edit_dir_update_error,
> + 0, 0, 0, 0, "..");
> + clear_bit(AFS_VNODE_DIR_VALID, &vnode->flags);
> + goto out;
> +}
> diff --git a/fs/afs/internal.h b/fs/afs/internal.h
> index 52aab09a32a9..c9d620175e80 100644
> --- a/fs/afs/internal.h
> +++ b/fs/afs/internal.h
> @@ -1073,6 +1073,8 @@ extern void afs_check_for_remote_deletion(struct afs_operation *);
> extern void afs_edit_dir_add(struct afs_vnode *, struct qstr *, struct afs_fid *,
> enum afs_edit_dir_reason);
> extern void afs_edit_dir_remove(struct afs_vnode *, struct qstr *, enum afs_edit_dir_reason);
> +void afs_edit_dir_update_dotdot(struct afs_vnode *vnode, struct afs_vnode *new_dvnode,
> + enum afs_edit_dir_reason why);
>
> /*
> * dir_silly.c
> diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h
> index 450c44c83a5d..a0aed1a428a1 100644
> --- a/include/trace/events/afs.h
> +++ b/include/trace/events/afs.h
> @@ -331,7 +331,11 @@ enum yfs_cm_operation {
> EM(afs_edit_dir_delete, "delete") \
> EM(afs_edit_dir_delete_error, "d_err ") \
> EM(afs_edit_dir_delete_inval, "d_invl") \
> - E_(afs_edit_dir_delete_noent, "d_nent")
> + EM(afs_edit_dir_delete_noent, "d_nent") \
> + EM(afs_edit_dir_update_dd, "u_ddot") \
> + EM(afs_edit_dir_update_error, "u_fail") \
> + EM(afs_edit_dir_update_inval, "u_invl") \
> + E_(afs_edit_dir_update_nodd, "u_nodd")
>
> #define afs_edit_dir_reasons \
> EM(afs_edit_dir_for_create, "Create") \
> @@ -340,6 +344,7 @@ enum yfs_cm_operation {
> EM(afs_edit_dir_for_rename_0, "Renam0") \
> EM(afs_edit_dir_for_rename_1, "Renam1") \
> EM(afs_edit_dir_for_rename_2, "Renam2") \
> + EM(afs_edit_dir_for_rename_sub, "RnmSub") \
> EM(afs_edit_dir_for_rmdir, "RmDir ") \
> EM(afs_edit_dir_for_silly_0, "S_Ren0") \
> EM(afs_edit_dir_for_silly_1, "S_Ren1") \
A few nits:
- There's an empty line with whitespace that I indicated above.
- I don't think your example in the commit message is quite sufficient
to show the problem; after a MakeDir the new directory will always get
fetched from the server, before or after the fix. You need to modify
it before the rename so that the local dir data is valid, for instance
by creating an additional file in "ccc".
That said the fix looks good and does fix the issue.
Marc
