xfs
[Top] [All Lists]

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Wed, 21 Aug 2013 14:15:20 -0700
Cc: Ben Myers <bpm@xxxxxxx>, Jan Kara <jack@xxxxxxx>, Abhijith Das <adas@xxxxxxxxxx>, xfs@xxxxxxxxxxx, linux-fsdevel <linux-fsdevel@xxxxxxxxxxxxxxx>, Steven Whitehouse <swhiteho@xxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130821181919.GA21378@xxxxxxxxxxxxx>
Organization: IBM
References: <1375828029-26360-1-git-send-email-sekharan@xxxxxxxxxx> <1375828029-26360-2-git-send-email-sekharan@xxxxxxxxxx> <20130821064357.GA8822@xxxxxxxxxxxxx> <20130821130152.GA9709@xxxxxxxxxxxxx> <20130821181247.GL5262@xxxxxxx> <20130821181919.GA21378@xxxxxxxxxxxxx>
Reply-to: sekharan@xxxxxxxxxx
Chrishtoph,

After we figured that there are ABI/API issues with adding pquota
information to the end (while using the same command), I posted what you
are suggesting now
(http://oss.sgi.com/archives/xfs/2013-07/msg00212.html) as I also do not
like redundant code. Please see Dave's comment at the link above in
which he wanted me to change the code so that the two commands are
totally independent. There was no objections to Dave's suggestion, so I
made the changes as he suggested.

On Wed, 2013-08-21 at 11:19 -0700, Christoph Hellwig wrote: 
> On Wed, Aug 21, 2013e at 01:12:47PM -0500, Ben Myers wrote:
> > So you don't like the addition of .get_xstatev in quotactl_ops, and
> > fs_quota_stat would need to match with fs_quota_statv, adding the project 
> > quota
> > to the end of the structure?
> 
> That was what I had in mind initially, before the additional
> complication were pointed out, except that we don't need it to look
> exactly the same - if we use put_user calls instead of copy_to_user that
> layout doesn't have to be the same.
> 
> However it seems like going down the stat route and having a kquota_info
> structure might be the better way to fully separate the in-kernel API
> from the userspace ABI.
> 
> > >   Well, the trouble is with gquota vs pquota - previously we report in
> > > qs_gquota field either group quotas or project quotas depending on what is
> > > turned on. Generic quota code doesn't know this so xfs get_xstatev() would
> > > have to recognize whether it is being called from the old Q_XGETSTAT
> > > quotactl or from the new Q_XGETSTATV quotactl to know where to fill in
> > > project quotas. And at that point you somewhat loose the elegancy of using
> > > one interface - we could set qs_version to some special value so that
> > > .get_xstatev() recognizes this and does the magic but that doesn't seem 
> > > very
> > > different from the extra call...
> > 
> > IIUC to make this happen without the addition of .get_xstate in 
> > quotactl_ops,
> > .get_xstate could also grow an argument to determine whether it was called 
> > as
> > Q_XGETSTAT vs Q_XGETSTATV.  If called as Q_XGETSTATV it can look at 
> > qs_version
> > to determine how much to copy.  That might be a bit cleaner than the 
> > qs_version
> > magic number, as long as you don't mind changing the .get_xstate interface.
> 
> I don't think we'd need that argument - the fs would always fill out
> both fields, then the implementation of Q_XGETSTAT would:
> 
>  (1) fail if both group and project quota information is present

There was a discussion on this issue and it was decided to provide back
only gquota information when both quotas are present and Q_XGETSTAT
command was used (instead of failing, which will break API)

> (2) export the pquota fields as gqouta if only project quota is present
> 
> > Anyway, please give a shout if I need to revert this.  I believe the commit
> > raced with the commentary.  ;)
> 
> As this is in-kernel only I don't think we need to revert anything, but
> it would be nice to fix it before 3.12 is released.  I didn't exactly
> race either, your reply to Jan made me look a it a bit more.
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 



<Prev in Thread] Current Thread [Next in Thread>