On Mon, Jul 01, 2013 at 10:50:30AM -0500, Chandra Seetharaman wrote:
> Hi Dave,
>
> I sent this email on 6/25 (just before my reply on 5/6 w.r.t adding the
> test to xfstests). Since I didn't get any response from you on this and
> got reply on the other one, I concluded that you accepted my
> justification/clarification.
>
> I just looked at the archives and do not see my original email :(... but
> is intact in my sent folder!!
>
> Please see if my justification below is valid.
>
> Chandra
>
> On Tue, 2013-06-25 at 16:31 +1000, Dave Chinner wrote:
> > On Sun, Jun 23, 2013 at 09:48:25PM -0500, Chandra Seetharaman wrote:
> > > Start using pquotino and define a macro to check if the
> > > superblock has pquotino.
> > >
> > > Keep backward compatibilty by alowing mount of older superblock
> > > with no separate pquota inode.
> > >
> > > Signed-off-by: Chandra Seetharaman <sekharan@xxxxxxxxxx>
> > > ---
> > > fs/xfs/xfs_fsops.c | 3 +-
> > > fs/xfs/xfs_mount.c | 51 +++++++++++++++++++++++++++++++--------
> > > fs/xfs/xfs_qm.c | 22 +++++++++--------
> > > fs/xfs/xfs_qm_syscalls.c | 24 ++++++++++++++++--
> > > fs/xfs/xfs_quota.h | 8 ------
> > > fs/xfs/xfs_sb.h | 16 +++++++++++-
> > > fs/xfs/xfs_super.c | 14 ++++++----
> > > include/uapi/linux/dqblk_xfs.h | 1 +
> > > 8 files changed, 99 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > index 614eb0c..3bf05f4 100644
> > > --- a/fs/xfs/xfs_fsops.c
> > > +++ b/fs/xfs/xfs_fsops.c
> > > @@ -501,7 +501,8 @@ xfs_growfs_data_private(
> > > error, agno);
> > > break;
> > > }
> > > - xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb, XFS_SB_ALL_BITS);
> > > + xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb,
> > > + xfs_sb_all_bits(&mp->m_sb));
> >
> > I think you could still pass XFS_SB_ALL_BITS to xfs_sb_to_disk(),
> > and do the XFS_SB_PQUOTINO filtering inside xfs_sb_quota_to_disk().
> >
> > Actually xfs_sb_all_bits() is only used here, and it seems to me
> > that it is not necessary as we do a pquota support check inside
> > xfs_sb_to_disk() and can handle this in that place...
>
> Yes, this is the only place that uses XFS_SB_ALL_BITS, and the purpose
> is to copy over the superblock to secondary.
>
> The change I made ensures that the caller does what is correct. i.e if
> it is copying an earlier version of superblock, it does not try to copy
> the pquotino, and if it uses the newer version of superblock it copies
> over the pquotino.
The caller does not need to know anything about how the quota bits
are encoded in the superblock.
> At some point (in my internal tree) I did have the way you suggested.
> But, doing it that way looked like a band-aid/hack, and felt this is the
> right way to do it (since the caller should provide appropriate
> information to the function).
The caller should not have to know anything about the specific
on-disk encoding the filesystem is currently using
That's the reason for encapsulating such on-disk format conversion
inside xfs_sb_to/from_disk(). Same with all the mount flags and so on
- the differences between what the code uses and what is on disk is
all encapsulated inside xfs_sb_quota_to/from_disk.
IOWs, from the outside, XFS_SB_ALL_BITS is all a caller needs to
pass into xfs_sb_to_disk() to get everything written to disk. Making
the caller have to be aware of the on-disk format when the function
being called is supposed to hide the details of the on-disk format
from the callers is, well, a bit silly.
> > > + if (*fields & XFS_SB_PQUOTINO) {
> > > + to->sb_gquotino = cpu_to_be64(from->sb_pquotino);
> > > + *fields &= ~XFS_SB_PQUOTINO;
> > > + }
> >
> > We will never do this because we've already cleared XFS_SB_PQUOTINO
> > before we entered xfs_sb_to_disk(). That doesn't seem right to me...
>
> Yes, if we are using a version of superblock where pquotino should not
> be used, we do not want to get into this block.
Why not? this code is executing the conversion from in-memory format
to the old on-disk format. This is exactly where such conversion
should be handled. This code looks right, but if you strip
XFS_SB_PQUOTINO at a high level, then it doesn't *ever* get used.
> If we are using PQUOTA in the superblock, that inode information will be
> in gquotino and will be converted over appropriately in
> xfs_sb_to_disk().
Sorry, I don't follow. You mean OQUOTA, yes?
Besides, all quota format conversions should be done in
xfs_sb_quota_to_disk(), not split between that function and
xfs_sb_to_disk()...
> > > @@ -201,8 +201,7 @@ xfs_qm_scall_quotaoff(
> > > /*
> > > * If quotas is completely disabled, close shop.
> > > */
> > > - if (((flags & XFS_MOUNT_QUOTA_ALL) == XFS_MOUNT_QUOTA_SET1) ||
> > > - ((flags & XFS_MOUNT_QUOTA_ALL) == XFS_MOUNT_QUOTA_SET2)) {
> > > + if ((flags & XFS_MOUNT_QUOTA_ALL) == XFS_MOUNT_QUOTA_ALL) {
> > > mutex_unlock(&q->qi_quotaofflock);
> > > xfs_qm_destroy_quotainfo(mp);
> > > return (0);
> >
> > This makes the assumption that userspace passes all three quota
> > types into the kernel in a single quota off call as the only way to
> > turn quotas off completely. What happens if they are turned off one
> > at a time? Shouldn't we detect when no more quotas are actually
> > enabled and then do this?
>
> Yes, I agree that there is a difference in behavior if the user turns
> off quota one-by-one Vs turns off all quota at once. But, the behavior
> difference is not seen by the user space. And, I did not make the
> change :)
>
> I just included project quota into the fold.
Sure, but my point still stands - if userspace currently expects
quota to be turned off by turning of user+group quota at the same
time, or user+project quota at the same time, then this will no
longer turn quotas off even if u/g or u/p were the only quotas
enabled. It's an unexpected change of behaviour, especially for
filesystems that don't support the separate pquota inode...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
