Re: [PATCH v9 4/6] xfs: Start using pquotaino from the superblock.

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

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@us.ibm.com>
> > > > > > ---
> > > > > >  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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs