xfs
[Top] [All Lists]

Re: [RFC v3 PATCH 1/4] xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [RFC v3 PATCH 1/4] xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS_OQUOTA_CHKD
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Fri, 17 Feb 2012 15:05:55 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120217183902.GE7762@xxxxxxx>
Organization: IBM
References: <20120123173158.31640.30333.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20120123173204.31640.53918.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20120211004112.GP7762@xxxxxxx> <1329149157.2213.55.camel@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20120217183902.GE7762@xxxxxxx>
Reply-to: sekharan@xxxxxxxxxx
Fine by me. Will make these changes in my next version.

But, the situation may not be temporary as we have to support
OQUOTA for backward compatibility reasons.

Chandra 

On Fri, 2012-02-17 at 12:39 -0600, Ben Myers wrote:
> Hey Chandra,
> 
> On Mon, Feb 13, 2012 at 10:05:57AM -0600, Chandra Seetharaman wrote:
> > On Fri, 2012-02-10 at 18:41 -0600, Ben Myers wrote:
> > 
> > <snip>
> > 
> > > > @@ -657,6 +686,7 @@ xfs_sb_to_disk(
> > > >  
> > > >                 fields &= ~(1LL << f);
> > > >         }
> > > > +       from->sb_qflags = saved_qflags;
> > > 
> > > I am disgusted that you should have to save the sb_qflags like this.
> > > After reading this 'fields' loop in xfs_sb_to_disk along with all the
> > > related crap I am actually feeling a little nauseous.
> > 
> > Let me know if you have any other solutions. I am open for it.
> 
> How would you feel about something like this?
> 
> @@ -622,6 +636,7 @@ xfs_sb_to_disk(
>         xfs_sb_field_t  f;
>         int             first;
>         int             size;GMCR
> +       __uint16_t      tmp16;
> 
>         ASSERT(fields);
>         if (!fields)
> @@ -636,6 +651,27 @@ xfs_sb_to_disk(
> 
>                 if (size == 1 || xfs_sb_info[f].type == 1) {
>                         memcpy(to_ptr + first, from_ptr + first, size);
> +               } else if (f == XFS_SBS_QFLAGS) {
> +                       /*
> +                        * The in-core version of sb_qflags do not have
> +                        * XFS_OQUOTA_* flags, whereas the on-disk version
> +                        * does.  Save the in-core sb_qflags temporarily,
> +                        * removing the new XFS_{PG}QUOTA_* flags and re-apply
> +                        * the old on-disk flags.  This is a temporary
> +                        * situation until the on-disk flags are updated.
> +                        */
> +                       tmp16 = from->sb_qflags &
> +                                       ~(XFS_PQUOTA_ENFD | XFS_PQUOTA_CHKD |
> +                                         XFS_GQUOTA_ENFD | XFS_GQUOTA_CHKD);
> +
> +                       if (from->sb_qflags &
> +                                       (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD))
> +                               tmp16 |= XFS_OQUOTA_ENFD;
> +                       if (from->sb_qflags &
> +                                       (XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD))
> +                               tmp16 |= XFS_OQUOTA_CHKD;
> +
> +                       *(__be16 *)(to_ptr + first) = cpu_to_be16(tmp16);
>                 } else {
>                         switch (size) {
>                         case 2:
> 
> This way we needn't worry about any change (or locking) of the incore
> superblock sb_qflags.
> 
> > > I don't know why in XFS we seem to feel the need to do everything in the
> > > most complicated way possible.
> 
> I suppose they were trying to cut down on conditionals on fields... maybe
> something that could be done better at compile time... someday.
> 
> Regards,
>       Ben
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 


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