On Wed, 2010-09-22 at 19:42 +0200, Arkadiusz MiÅkiewicz wrote:
> This patch adds support for 32bit project quota identificators.
>
> On disk format is backward compatible with 16bit projid numbers. projid
> on disk is now keept in two 16bit values - di_projid_lo (which holds the
> same position as old 16bit projid value) and new di_projid_hi (takes
> existing padding) and convertes from/to 32bit value on the fly.
>
> PROJID32BIT feature2 flag is set automaticly when trying to use 32bit
> quota project identificator.
>
> Signed-off-by: Arkadiusz MiÅkiewicz <arekm@xxxxxxxx>
> ---
Sorry it took me so long to review this. I have some feedback.
I think what you've done looks generally good. The
only issues are related to the new feature bit. I also
wonder whether there is *any* chance the (formerly pad)
projid_hi field contains anything other than 0 on disk for
any existing filesystems.
First of all, I'll say that I think the superblock flag
should be set only if it is known the file system has
(at one time) held at least one project id requiring
more than 16 bits to represent. Even if 32-bit project
ids are supported, the flag should not be set on the
superblock if no project id >= 2^16 is present.
The way your code is now, in xfs_ioctl_setattr() no longer
returns EINVAL when a passed-in project exceeds that
representable in 16 bits. Instead, if the passed-in value
is >= 2^16 you force the superblock to have the new
PROJID32BIT feature flag turned on. (This is consistent
with what I suggest, above.)
But I don't agree with setting that flag in all cases,
even when a 32-bit project id value is supplied. I could
envision, for example, someone wanting to avoid exceeding
the 16 bit limit to ensure their file system remains
compatible with the older format. On the other hand,
automatically setting it is useful as well.
Instead, think it would be good to have a mount flag that
indicates whether 32-bit project ids are to be supported
(default: not supported). If the superblock read in at
mount time had PROJID32BIT set, but the mount options did
not indicate 32-bit project id support, the mount should
fail with an explanation. (Without enabling the mount
flag we would not handle large project ids properly).
If the mount flag were supplied but the superblock did not
(yet) have the flag set, that's OK. If any project id that
required > 16 bits to represent got recorded, the superblock
feature bit would be set to indicate that (as you propose).
Matching mount and superblock flags would of course be a
supported configuration.
If the mount flag is set, then >= 2^16 project ids would
be accepted in xfs_ioctl_setattr() (as you now do), but
if it is not set it would not be allowed (as the existing
code does).
The mount flag could furthermore affect how the projid_hi
superblock field is interpreted. If the 32-bit project id
mount flag is not set, then the projid_hi would be forced
to 0 when read off the disk and asserted 0 when writing
to the disk. Similarly, if the superblock flag indicated
no >16-bit project ids were present, their projid_hi
values would be forced to 0 (even if the mount flag
indicated 32-bit support).
It may well be that XFS has always and consistently
written 0's in the pad fields for the affected structures
here. And if so, there's no reason to be concerned
that any existing filesystems have non-zero garbage
that would have any adverse effect. The "forced zero"
handling I mentioned above would address most of these
cases. If there is any risk of non-zero values in
the projid_hi location we may need a utility of some
kind to fix that before allowing a filesystem to be
mounted with >16 bit projid support.
OK, enough of that. I have a few other specific comments
related to your actual code, below.
-Alex
> What has changed?
> - sb_bad_features2 is also updated
> - new helper - xfs_addprojid32bit
> - drop 16bit projid protection in latest kernels
>
> I think it's ready to merge but it lacks final Reviewed-by,
> Tested-by (beside me) etc :-(
. . .
> diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c
> b/fs/xfs/linux-2.6/xfs_ioctl.c
> index 4fec427..aa72465 100644
> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
. . .
> @@ -953,13 +946,22 @@ xfs_ioctl_setattr(
> goto error_return;
> }
>
> - /*
> - * Do a quota reservation only if projid is actually going to
> change.
> - */
> if (mask & FSX_PROJID) {
> + /*
> + * Switch on the PROJID32BIT superblock bit when
> needed
> + * (implies also FEATURES2)
> + */
> + if (!xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb)
> &&
> + fa->fsx_projid > (__uint16_t)-1)
Don't bother checking the version here, you do that again in the
xfs_addprojid32bit() helper. Just check for exceeding the limit.
> + xfs_addprojid32bit(tp, ip);
> +
. . .
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 0898c54..5bbb100 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
. . .
> @@ -335,6 +336,23 @@ xfs_iflags_test_and_clear(xfs_inode_t *ip, unsigned short flags)
> }
>
> /*
> + * Project quota id helpers
> + */
> +static inline prid_t
> +xfs_get_projid(xfs_inode_t *ip)
> +{
> + return (prid_t)(ip->i_d.di_projid_hi) << 16 | ip->i_d.di_projid_lo;
No need for the parentheses around ip->i_d.di_projid_hi here.
> +}
> +
> +static inline void
> +xfs_set_projid(xfs_inode_t *ip,
> + prid_t projid)
> +{
> + ip->i_d.di_projid_hi = (__uint16_t) (projid >> 16);
> + ip->i_d.di_projid_lo = (__uint16_t) (projid & 0xffff);
> +}
> +
> +/*
> * Manage the i_flush queue embedded in the inode. This completion
> * queue synchronizes processes attempting to flush the in-core
> * inode back to disk.
. . .
> diff --git a/fs/xfs/xfs_sb.h b/fs/xfs/xfs_sb.h
> index 1b017c6..f7674c4 100644
> --- a/fs/xfs/xfs_sb.h
> +++ b/fs/xfs/xfs_sb.h
. . .
> @@ -495,6 +497,19 @@ static inline void xfs_sb_version_removeattr2(xfs_sb_t *sbp)
> sbp->sb_versionnum &= ~XFS_SB_VERSION_MOREBITSBIT;
> }
>
> +static inline int xfs_sb_version_hasprojid32bit(xfs_sb_t *sbp)
> +{
> + return xfs_sb_version_hasmorebits(sbp) &&
> + (sbp->sb_features2 & XFS_SB_VERSION2_PROJID32BIT);
> +}
> +
> +static inline void xfs_sb_version_addprojid32bit(xfs_sb_t *sbp)
> +{
> + sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
MOREBITSBIT is only meaningful for superblock version 4.
So you should first convert this field to version 4.
You might use using something like this (but setting
the extra bits may be more than you want):
sbp->sb_versionnum = xfs_sb_version_tonew(sbp->sb_versionnum);
sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
(It looks like xfs_sb_version_addattr2() should have been
written that way also.)
> + sbp->sb_features2 |= XFS_SB_VERSION2_PROJID32BIT;
> + sbp->sb_bad_features2 |= XFS_SB_VERSION2_PROJID32BIT;
There is no need to set the sb_bad_features2 field. None
of the other version functions do this. It is fixed when
the file system is mounted if necessary.
> +}
> +
> /*
> * end of superblock version macros
> */
. . .
> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index 4c7c7bf..72319a9 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
. . .
> @@ -1978,9 +1978,9 @@ xfs_symlink(
>
> udqp = gdqp = NULL;
> if (dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
> - prid = dp->i_d.di_projid;
> + prid = xfs_get_projid(dp);
> else
> - prid = (xfs_prid_t)dfltprid;
> + prid = dfltprid;
I know this isn't your doing, but "dfltprid" is a complete CRAP
symbol name, especially one that should be constrained to the XFS
scope. How about renaming this XFS_PROJID_DEFAULT or something?
> /*
> * Make sure that we have allocated dquot(s) on disk.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
