On Tue, 2013-07-09 at 11:21 +1000, Dave Chinner wrote:
> On Mon, Jul 08, 2013 at 06:20:30PM -0500, Chandra Seetharaman wrote:
> > On Thu, 2013-07-04 at 11:12 +1000, Dave Chinner wrote:
> > > 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.
> >
> > Caller doesn't need to know about quota bits, that is why I encapsulated
> > it into the function xfs_sb_all_bits().
>
> Requiring a function at the caller site to filter bits out is saying
> "the caller needs to know it has to filter certain options out".
>
> What happens if we add a new piece of optional functionality that is
> conditional for superblock writeback? So sometimes we need to filter
> only PQUOTINO_BIT, sometimes we nee dto filter FOO_BIT, sometimes
> we need to filter BAR_BIT, and so on?
>
> What you are proposing effectively sets us down the path of having
> to do this in future:
>
> if (xfs_sb_version_hasfoo)
> xfs_sb_to_disk(XFS_SB_FOO_BIT)
> if (xfs_sb_version_hasbar)
> xfs_sb_to_disk(XFS_SB_BAR_BIT)
> xfs_sb_to_disk((XFS_SB_ALL_BITS & ~(XFS_SB_FOO_BIT|XFS_SB_BAR_BIT)));
>
> All this needs to be handled in xfs_sb_to_disk().
>
> > > 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.
> >
> > Encapsulation in xfs_sb_to_disk() is not enough for this situation. If
> > both PQUOTINO and GQUOTINO is set for version 4 of the superblock,
> > xfs_sb_to_disk will not be able to know if it has to treat PQUOTINO as
> > valid or GQUOTINO as valid.
>
> The contents of from->sb_qflags tells you which one is valid.
> Indeed, you have to look at from->sb_qflags to set the OQUOTA flags
> correctly in to->sb_qflags, so I don't see why you can't use
> from->sb_qflags to determine which of G/PQUOTINO needs to copied
> across to to->sb_gquotino....
>
Ok. That is a good option. So, this is what I am thinking that I will
add before the PQUOTINO check in xfs_sb_quota_to_disk():
if ((*fields & XFS_SB_PQUOTINO) && (*fields & XFS_SB_GQUOTINO)) {
/*
* PQUOTINO and GQUOTINO cannot be used together in versions
* earlier than version 5 of superblock. If used, use sb_flags
* to resolve which one should be set.
*/
if (from->sb_qflags & XFS_PQUOTA_ACCT)
*fields &= ~XFS_SB_GQUOTINO;
else
*fields &= ~XFS_SB_PQUOTINO;
}
> > > > > > /*
> > > > > > * 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...
> >
> > IIUC, user space will not see any difference in behavior even if this
> > specific code block is not present in this function. Whatever is being
> > done in xfs_qm_destroy_quotainfo() is done by the code just below the
> > above block in xfs_qm_scall_quotaoff(). Only additional thing
> > xfs_qm_destroy_quotainfo() does is to clean up all other in-kernel data
> > structures associated with quota for this mount point.
> >
> > The behavior was that if _all_ the quotas are turned off at the same
> > time, clean up all in-kernel data structures associated with quota. If
> > the quotas were turned off one at a time, leave the in-kernel stuff even
> > when the last quota is turned off.
> >
> > I maintained the same behavior.
>
> That's where I disagree. You kept the same /line of reasoning/ as the
> original code, not the same behaviour. What I'm saying is that
> the line of reasoning is wrong to begin with, and hence keeping it
> is not the right thing to do.
>
> Indeed, code archeology shows this line of reasoning is zero-day
> Irix behaviour:
>
> http://oss.sgi.com/cgi-bin/gitweb.cgi?p=archive/xfs-import.git;a=blob;f=fs/xfs/xfs_qm_syscalls.c;hb=0d5ad8383061fbc0a9804fbb98218750000fe032
>
> 730 /*
> 731 * If quotas is completely disabled, close shop.
> 732 */
> 733 if ((flags & XFS_MOUNT_QUOTA_ALL) == XFS_MOUNT_QUOTA_ALL) {
> 734 mutex_unlock(&mp->QI_QOFFLOCK);
> 735 xfs_qm_destroy_quotainfo(mp);
> 736 return (0);
> 737 }
>
> Hmmm, looks kind of familiar, doesn't it? But what's the caller
> context?
>
> 155 case Q_QUOTAOFF:
> 156 /* */
> 157 flags = addr?
> 158 xfs_qm_import_flags((uint *)addr) :
> 159 XFS_ALL_QUOTA_ACCT_ENFD;
> 160 error = xfs_qm_scall_quotaoff(mp, flags);
> 161
>
> And from xfs_dquot.h:
>
> 95 #define XFS_ALL_QUOTA_ACCT_ENFD (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD|\
> 96 XFS_PQUOTA_ACCT|XFS_PQUOTA_ENFD)
>
> If no parameter was passed, the default behaviour is to turn off
> all quotas. IOWs, this logic made sense Irix, but the linux case has no
> such default behaviour - it only passes what userspace says to turn
> off.
>
> And that's exactly my point - these semantics made sense for Irix all
> those years ago, but they simply don't make sense now. Hence while
> we are touching this code we should fix it up. i.e. if we've turned
> off all the quotas, clean up properly.
In effect, you are asking me to fix a problem that existed when xfs was
ported to Linux.
Sure, will do it.
I will send a separate patch to fix the problem and change my patch
accordingly.
>
> > I understand your point that _all_ mean u/g or u/p in older version of
> > superblock. But, the difference in behavior is not drastic and is not
> > seen by the user space at all.
> >
> > If still unconvinced, is this the code you want to see there ?
> >
> > if (((flags & XFS_MOUNT_QUOTA_ALL) == XFS_MOUNT_QUOTA_ALL) &&
> > xfs_sb_version_has_pquota(&mp->sb)) ||
> > ((flags & XFS_MOUNT_QUOTA_ALL) == XFS_MOUNT_QUOTA_SET1) ||
> > ((flags & XFS_MOUNT_QUOTA_ALL) == XFS_MOUNT_QUOTA_SET2)) {
> > :
> > :
> > }
>
> No. If, as a result of turning off quotas, there are no active
> quotas then turn off the quota subsystem. mp->m_qflags will tell us
> exactly what quota is still running when we get to this point...
>
> Cheers,
>
> Dave.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
