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

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

[Chandra Seetharaman]

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@us.ibm.com>
---
 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;
+
 	if (sbp->sb_qflags & XFS_OQUOTA_ENFD)
 		sbp->sb_qflags |= (sbp->sb_qflags & XFS_PQUOTA_ACCT) ?
 					XFS_PQUOTA_ENFD : XFS_GQUOTA_ENFD;
-- 
1.7.1



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Carlos Maiolino]

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@us.ibm.com>
> ---
>  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;
> +
>  	if (sbp->sb_qflags & XFS_OQUOTA_ENFD)
>  		sbp->sb_qflags |= (sbp->sb_qflags & XFS_PQUOTA_ACCT) ?
>  					XFS_PQUOTA_ENFD : XFS_GQUOTA_ENFD;
> -- 
> 1.7.1
> 
Hi,

in this case, wouldn't be better to make mkfs initialize the quota inodes with
NULLFSINO instead of need to convert it into kernel?

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Chandra Seetharaman]

On Fri, 2013-07-12 at 11:27 -0300, Carlos Maiolino 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@us.ibm.com>
> > ---
> >  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;
> > +
> >  	if (sbp->sb_qflags & XFS_OQUOTA_ENFD)
> >  		sbp->sb_qflags |= (sbp->sb_qflags & XFS_PQUOTA_ACCT) ?
> >  					XFS_PQUOTA_ENFD : XFS_GQUOTA_ENFD;
> > -- 
> > 1.7.1
> > 
> Hi,
> 
> in this case, wouldn't be better to make mkfs initialize the quota inodes with
> NULLFSINO instead of need to convert it into kernel?

Hi Carlos,

Yes. I will send a patch for mkfs also (to initialize it to NULLFSINO).

We still need the kernel fix for the already existing filessytems.

Chadnra
> 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

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@us.ibm.com>
> ---
>  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.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Chandra Seetharaman]

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@us.ibm.com>
> > ---
> >  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 ?

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

> 
> Cheers,
> 
> Dave.


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

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@us.ibm.com>
> > > ---
> > >  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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Chandra Seetharaman]

On Tue, 2013-07-16 at 10:23 +1000, Dave Chinner wrote:
> 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@us.ibm.com>
> > > > ---
> > > >  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.

How about this:

/*
 * older mkfs doesn't initialize quota inodes to NULLFSINO
 * This leads to in-core values having two different values
 * for a quota inode to be invalid: 0 and NULLFSINO.
 * Change it to a single value NULLFSINO.
 *
 * Note that this change affect only the in-core values. 
 * These values are not written back to disk unless any
 * quota information is written to the disk. Even in that
 * case, sb_pquotino field is not written to disk unless
 * the superblock supports pquotino.
 */

> Cheers,
> 
> Dave.


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

On Wed, Jul 17, 2013 at 04:42:10PM -0500, Chandra Seetharaman wrote:
> On Tue, 2013-07-16 at 10:23 +1000, Dave Chinner wrote:
> > 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@us.ibm.com>
> > > > > ---
> > > > >  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.
> 
> How about this:
> 
> /*
>  * older mkfs doesn't initialize quota inodes to NULLFSINO
>  * This leads to in-core values having two different values
>  * for a quota inode to be invalid: 0 and NULLFSINO.
>  * Change it to a single value NULLFSINO.
>  *
>  * Note that this change affect only the in-core values. 
>  * These values are not written back to disk unless any
>  * quota information is written to the disk. Even in that
>  * case, sb_pquotino field is not written to disk unless
>  * the superblock supports pquotino.
>  */

Nice explanation. Expand it out to 80 columns and it's golden :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs