From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 2/2] xfs: reserve quota for target dir expansion when renaming files
Date: Thu, 10 Mar 2022 09:05:53 +1100 [thread overview]
Message-ID: <20220309220553.GI661808@dread.disaster.area> (raw)
In-Reply-To: <164685375248.495923.9228795379646460264.stgit@magnolia>
On Wed, Mar 09, 2022 at 11:22:32AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> XFS does not reserve quota for directory expansion when renaming
> children into a directory. This means that we don't reject the
> expansion with EDQUOT when we're at or near a hard limit, which means
> that unprivileged userspace can use rename() to exceed quota.
>
> Rename operations don't always expand the target directory, and we allow
> a rename to proceed with no space reservation if we don't need to add a
> block to the target directory to handle the addition. Moreover, the
> unlink operation on the source directory generally does not expand the
> directory (you'd have to free a block and then cause a btree split) and
> it's probably of little consequence to leave the corner case that
> renaming a file out of a directory can increase its size.
>
> As with link and unlink, there is a further bug in that we do not
> trigger the blockgc workers to try to clear space when we're out of
> quota.
>
> Because rename is its own special tricky animal, we'll patch xfs_rename
> directly to reserve quota to the rename transaction.
Yeah, and this makes it even more tricky - the retry jumps back
across the RENAME_EXCHANGE callout/exit from xfs_rename. At some
point we need to clean up the spaghetti that rename has become.
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> fs/xfs/xfs_inode.c | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
>
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index a131bbfe74e4..8ff67b7aad53 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3095,7 +3095,8 @@ xfs_rename(
> bool new_parent = (src_dp != target_dp);
> bool src_is_directory = S_ISDIR(VFS_I(src_ip)->i_mode);
> int spaceres;
> - int error;
> + bool retried = false;
> + int error, space_error;
>
> trace_xfs_rename(src_dp, target_dp, src_name, target_name);
>
> @@ -3119,9 +3120,12 @@ xfs_rename(
> xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip, wip,
> inodes, &num_inodes);
>
> +retry:
> + space_error = 0;
> spaceres = XFS_RENAME_SPACE_RES(mp, target_name->len);
> error = xfs_trans_alloc(mp, &M_RES(mp)->tr_rename, spaceres, 0, 0, &tp);
> if (error == -ENOSPC) {
> + space_error = error;
nospace_error.
> spaceres = 0;
> error = xfs_trans_alloc(mp, &M_RES(mp)->tr_rename, 0, 0, 0,
> &tp);
> @@ -3175,6 +3179,31 @@ xfs_rename(
> target_dp, target_name, target_ip,
> spaceres);
>
> + /*
> + * Try to reserve quota to handle an expansion of the target directory.
> + * We'll allow the rename to continue in reservationless mode if we hit
> + * a space usage constraint. If we trigger reservationless mode, save
> + * the errno if there isn't any free space in the target directory.
> + */
> + if (spaceres != 0) {
> + error = xfs_trans_reserve_quota_nblks(tp, target_dp, spaceres,
> + 0, false);
> + if (error == -EDQUOT || error == -ENOSPC) {
> + if (!retried) {
> + xfs_trans_cancel(tp);
> + xfs_blockgc_free_quota(target_dp, 0);
> + retried = true;
> + goto retry;
> + }
> +
> + space_error = error;
> + spaceres = 0;
> + error = 0;
> + }
> + if (error)
> + goto out_trans_cancel;
> + }
> +
> /*
> * Check for expected errors before we dirty the transaction
> * so we can return an error without a transaction abort.
> @@ -3215,6 +3244,8 @@ xfs_rename(
> */
> if (!spaceres) {
> error = xfs_dir_canenter(tp, target_dp, target_name);
> + if (error == -ENOSPC && space_error)
> + error = space_error;
And move this error transformation to out_trans_cancel: so it only
has to be coded once.
Other than that, it's about as clean as rename allows it to be right
now.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2022-03-09 22:05 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-09 19:22 [PATCHSET v2 0/2] xfs: make quota reservations for directory changes Darrick J. Wong
2022-03-09 19:22 ` [PATCH 1/2] xfs: reserve quota for dir expansion when linking/unlinking files Darrick J. Wong
2022-03-09 21:48 ` Dave Chinner
2022-03-09 23:33 ` Darrick J. Wong
2022-03-10 1:50 ` Dave Chinner
2022-03-09 19:22 ` [PATCH 2/2] xfs: reserve quota for target dir expansion when renaming files Darrick J. Wong
2022-03-09 22:05 ` Dave Chinner [this message]
2022-03-09 23:36 ` Darrick J. Wong
2022-03-10 21:53 [PATCHSET v3 0/2] xfs: make quota reservations for directory changes Darrick J. Wong
2022-03-10 21:53 ` [PATCH 2/2] xfs: reserve quota for target dir expansion when renaming files Darrick J. Wong
2022-03-10 22:28 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220309220553.GI661808@dread.disaster.area \
--to=david@fromorbit.com \
--cc=djwong@kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).