xfs
[Top] [All Lists]

Re: [PATCH] xfs: Initialize all quota inodes to be NULLFSINO

To: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Subject: Re: [PATCH] xfs: Initialize all quota inodes to be NULLFSINO
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 16 Jul 2013 10:23:52 +1000
Cc: XFS mailing list <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1373928885.20769.42.camel@xxxxxxxxxxxxxxxxxx>
References: <1373593665.20769.10.camel@xxxxxxxxxxxxxxxxxx> <20130713021349.GH3438@dastard> <1373928885.20769.42.camel@xxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Jul 15, 2013 at 05:54:45PM -0500, Chandra Seetharaman wrote:
> On Sat, 2013-07-13 at 12:13 +1000, Dave Chinner wrote:
> > On Thu, Jul 11, 2013 at 08:47:45PM -0500, Chandra Seetharaman wrote:
> > > 
> > > mkfs doesn't initialize the quota inodes to NULLFSINO as it
> > > does for the other internal inodes. This leads to check for two
> > > values (0 and NULLFSINO) to make sure if a quota inode is
> > > valid.
> > > 
> > > Solve that problem by initializing the values to NULLFSINO
> > > if they are 0.
> > > 
> > > Note that these values are not written back to on-disk
> > > superblock unless some quota is enabled on the filesystem.
> > > 
> > > Signed-off-by: Chandra Seetharaman <sekharan@xxxxxxxxxx>
> > > ---
> > >  fs/xfs/xfs_mount.c |   12 ++++++++++++
> > >  1 files changed, 12 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > index 2b0ba35..8b95933 100644
> > > --- a/fs/xfs/xfs_mount.c
> > > +++ b/fs/xfs/xfs_mount.c
> > > @@ -572,6 +572,18 @@ out_unwind:
> > >  static void
> > >  xfs_sb_quota_from_disk(struct xfs_sb *sbp)
> > >  {
> > > + /*
> > > +  * older mkfs doesn't initialize quota inodes to NULLFSINO,
> > > +  * which leads to two values for a quota inode to be invalid:
> > > +  * 0 and NULLFSINO. Fix it.
> > > +  */
> > > + if (sbp->sb_uquotino == 0)
> > > +         sbp->sb_uquotino = NULLFSINO;
> > > + if (sbp->sb_gquotino == 0)
> > > +         sbp->sb_gquotino = NULLFSINO;
> > > + if (sbp->sb_pquotino == 0)
> > > +         sbp->sb_pquotino = NULLFSINO;
> > 
> > There coment needs to point out that this is changing the in-memory
> > superblock value right here, so we don't need to make changing
> > sb_pquotino dependent on the superblock having support for this
> > feature.
> 
> But, we are not just changing just for sb_pquotino, right ?

Sure, but those other two fields are always present in the
superblock...

> Since we are changing all the fields independent of superblock
> supporting sb_pquotino, I thought the comment should be generic.

So why is it safe to modify this when sb_pquotino isn't valid in the
superblock (i.e. v4 superblock) and the only valid on-disk value for
an unused section of the superblock is zero?

That's what the comment needs to explain.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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