Re: [PATCH 14/15] xfs: dquot log reservations are too small

[Chandra Seetharaman <sekharan@us.ibm.com>]

some nits below on commit log and comments.

otherwise consider

Reviewed-by: Chandra Seethraman <sekharan@us.ibm.com>

On Thu, 2013-06-27 at 16:04 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> During review of the separate project quota inode patches, it bcame
became(typo)

> obvious that the dquot log reservation calculation underestimated
> the number dquots that can be modified in a transaction. This has
> it's roots way back in the Irix quota implementation.
> 
> That is, when quotas were first implemented in XFS, it only
> supported user and project quotas as Irix did not have group quotas.
> Hence the worst case operation involving dquot modification was
> calculated to involve 2 user dquots and 1 project dquot or 1 user
> dequot and 2 project dquots. i.e. 3 dquots. This was determined back
> in 1996, and has remained unchanged ever since.

How about the missing reservation for the log format header ? It is a
day one problem, right ?

> 
> However, back in 2001, the Linux XFS port dropped all support for
> project quota and implmented group quotas over the top. This was

implemented(typo)

> effectively done with a search-and-replace of project with group,
> and as such the log reservation was not changed. However, with the
> advent of group quotas, chmod and rename now could modify more than
> 3 dquots in a single transaction - both could modify 4 dquots. Hence
> this log reservation has been wrong for a long time.
> 
> In 2005, project quotas were reintroduced into Linux, but they were

introduced ? (it was mentioned above that 2001 Linux port removed
project quota, so it never existed in Linux ?!)

> implemented to be mutually exclusive to group quotas, and so this
> didn't add any new changes to the dquot log reservation. hence when
> project quotas were in use, everything was still fine, just like
> in the Irix days.

you can say that... but, when the group quota is used the problem
mentioned above exists.

> 
> Now, with the addition of the separate project quota inode, group
> and project quotas are no longer mutually exclusive, and hence
> operations can now modify three dquots per inode where previously it
> was only two. The worst case here is the rename transaction, which
> can allocate/free space on two different directory inodes, and if
> they have different uid/gid/prid configurations and are world
> writeable, then rename can actually modify 6 different dquots now.
> 
> Further, the dquot log reservation doesn't take into account the
> space used by the dquot log format structure that preceeds the dquot

precedes(typo)

> that is logged, and hence further underestimates the worst case
> log space required by dquots during a transaction.
> 
> Hence the worst case log reservation needs to be increased from 3 to
> 6, and it needs to take into account a log format header for each of
> those dquots.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_quota.h       |   26 ++++++++++++++++++++++----
>  fs/xfs/xfs_trans_dquot.c |    9 ++++-----
>  2 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
> index c38068f..aa4ec5a 100644
> --- a/fs/xfs/xfs_quota.h
> +++ b/fs/xfs/xfs_quota.h
> @@ -108,11 +108,29 @@ typedef struct xfs_dqblk {
>  	{ XFS_DQ_FREEING,	"FREEING" }
> 
>  /*
> - * In the worst case, when both user and group quotas are on,
> - * we can have a max of three dquots changing in a single transaction.
> + * We have the possibility of all three quota types being active at once,
> + * and hence free space modification requires modify all three current dquots in

s/requires modify all/requires modification of all/ ?

> + * a single transaction.  For this case, we need to have a reservation of at
> + * least 3 dquots.
> + *
> + * However, a chmod operation can change both UID and GID in the one

s/the// ?

> + * transaction, resulting in requiring {old, new} x {uid, gid} dquots to be
> + * modified. This means we need to reserve space for at least 4 dquots are the

s/are/for/ ?
> + * worst case reservation.

This not the worst case (since the next case is the worst case),
correct ?

We can just say "for this case". what do you think ?

> + *
> + * And in the worst case, there's a rename operation that can be modifying up to
> + * 4 inodes with dquots attached to them. In reality, the only inodes that can
> + * have their dquots modified are the source and destination directory inodes
> + * due to directory name creation and removal. That can require space allocation
> + * and/or freeing on both directory inodes, and hence all three dquots on each
> + * inode can be modified. And if the directories are world writeable, all the
> + * dquots can be unique and so 6 dquots can be modified....
> + *
> + * And, of course, we also need to take into account the dquot log format item
> + * used to describe each dquot.
>   */
> -#define XFS_DQUOT_LOGRES(mp)	(sizeof(xfs_disk_dquot_t) * 3)
> -
> +#define XFS_DQUOT_LOGRES(mp)	\
> +	((sizeof(struct xfs_dq_logformat) + sizeof(struct xfs_disk_dquot)) * 6)
> 
>  /*
>   * These are the structures used to lay out dquots and quotaoff
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index fec75d0..076cc25 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -292,11 +292,10 @@ xfs_trans_mod_dquot(
> 
> 
>  /*
> - * Given an array of dqtrx structures, lock all the dquots associated
> - * and join them to the transaction, provided they have been modified.
> - * We know that the highest number of dquots (of one type - usr OR grp),
> - * involved in a transaction is 2 and that both usr and grp combined - 3.
> - * So, we don't attempt to make this very generic.
> + * Given an array of dqtrx structures, lock all the dquots associated and join
> + * them to the transaction, provided they have been modified.  We know that the
> + * highest number of dquots of one type - usr, grp OR prj - involved in a
> + * transaction is 2 so we don't need to make this very generic.

much clear now.
>   */
>  STATIC void
>  xfs_trans_dqlockedjoin(


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs