xfs
[Top] [All Lists]

Re: [PATCH 1/4] xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS_OQUOTA

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/4] xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS_OQUOTA_CHKD
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Tue, 17 Jul 2012 20:44:17 +0800
Cc: xfs@xxxxxxxxxxx, Ben Myers <bpm@xxxxxxx>, Mark Tinguely <tinguely@xxxxxxx>, Chris Mason <chris.mason@xxxxxxxxxx>, Chandra Seetharaman <sekharan@xxxxxxxxxx>
In-reply-to: <20120717073221.GA24395@xxxxxxxxxxxxx>
Organization: Oracle
References: <4FFFDDB8.6000406@xxxxxxxxxx> <20120717073221.GA24395@xxxxxxxxxxxxx>
Reply-to: jeff.liu@xxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.28) Gecko/20120313 Thunderbird/3.1.20
On 07/17/2012 03:32 PM, Christoph Hellwig wrote:

>> +    if ((to->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) &&
>> +        (to->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
>> +                          XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD))) {
>> +            xfs_notice(mp, "Super block has XFS_OQUOTA bits along with "
>> +                    "XFS_PQUOTA and/or XFS_GQUOTA bits. Fixing it.\n");
>> +    }
>> +    if (to->sb_qflags & XFS_OQUOTA_ENFD)
>> +            to->sb_qflags |= (to->sb_qflags & XFS_PQUOTA_ACCT) ?
>> +                                    XFS_PQUOTA_ENFD : XFS_GQUOTA_ENFD;
>> +    if (to->sb_qflags & XFS_OQUOTA_CHKD)
>> +            to->sb_qflags |= (to->sb_qflags & XFS_PQUOTA_ACCT) ?
>> +                                    XFS_PQUOTA_CHKD : XFS_GQUOTA_CHKD;
>> +    to->sb_qflags &= ~(XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD);
> 
> It's suggest factoring this into a xfs_quota_flags_from_disk helper,
> which a) makes this code more clear, and b) can get a comment about what
> is going on.

Exactly!

> 
>> +                    /*
>> +                     * 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.
>> +                     */
>> +                    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_CHKD;
>> +                    *(__be16 *)(to_ptr + first) = cpu_to_be16(tmp16);
> 
> Same here for the other direction.

Ok, I'll take care of it as well.

> 
>> + * Start differentiating group quota and project quota in-core
>> + * using distinct flags, instread of using the combined OQUOTA flags.
>> + *
>> + * Conversion to and from the combined OQUOTA flag (if necessary)
>> + * is done only in xfs_sb{to, from}_disk()
>> + */
>> +#define XFS_GQUOTA_ENFD     0x0080  /* group quota limit enforced */
>> +#define XFS_GQUOTA_CHKD 0x0100      /* quotacheck run on group quotas */
>> +#define XFS_PQUOTA_ENFD     0x0200  /* project quota limits enforced */
>> +#define XFS_PQUOTA_CHKD     0x0800  /* quotacheck run on project quotas.
>> +                             * FIXME: here we cannot using 0x0400
>> +                             * because it is already reserved for
>> +                             * XFS_GQUOTA_ACTIVE.  Otherwise,
>> +                             * XFS_IS_GQUOTA_ON() is also true
>> +                             * although it does not enabled.
>> +                             */
> 
> Seems like all te 0x0?00 values are used for XFS_U?UOTA_ACTIVE flags.
> Why not move it to the 0x?000 range?

I have not took that previously because it will make the code looks a bit 
in-continuous, consider we have to skip all 0x0?00.
But yes, move PQUOTA_ENFD/PQUOTA_CHKD to 0x?000 range is fine, thanks for the 
advice!

-Jeff
 .

> 
> Otherwise these changes look good to me.


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