On Thu, Sep 23, 2010 at 05:51:01PM -0500, Alex Elder wrote:
> 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.
It will be. converion between v1 and v2 inodes has always zeroed the
padding, and creation of inodes always zeros the entire inode before
stamping it with the inode template. hence we can assume that the
padding is always zero.
....
> 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).
Too much complexity, and a mount option that needs to be supported
forever. If the admin has to set a mount option, it's just as easy to
ask them to run xfs_admin, and then we don't have a bunch of extra
code and complex mount time logic to maintain forever.
> > + 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.
All XFS filesystems created since Irix 6.2 are version 4 filesystems,
including every filesystem ever made on linux. The setting of the
XFS_SB_VERSION_MOREBITSBIT like this is just fine.
> 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.
Actually, there is. The attr2 code is broken w.r.t. to this and needs
complex logic during mount to ensure it isn't lost once set. Setting
the sb_bad_features2 here means that we don't need to rely on other
code to fix up the fact we didn't keep the sb_features2 and
sb_bad_features2 fields consistent. Indeed, if we clear bits, we
cannot rely on the mount code to fix that up, because it only
detects missing bits and sets them again...
> None
> of the other version functions do this.
Because none of them (except attr2) dynamically set feature2 bits.
All the userspace code has been fixed to ensure that we when we set
a feature2 bit, we also set the bad_features2 bit.
> It is fixed when
> the file system is mounted if necessary.
That was introduced to try to fix up the mess of bad userspace tools
writing to the wrong features2 field. attr2 handling is a mess
because of this. We should not be writing new code that relies on
the mount code to fix up mismatches, and we should be
ensuring/fixing all the kernel code that sets/clears features2 bits
to also sets/clears the bad_features2 bits as well.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
