xfs: Don't keep silent if sunit/swidth can not be changed via mount

From: Jie Liu<[email protected]>

As per the mount man page, sunit and swidth can be changed via mount options. For XFS, on the face of it, those options seems works if the specified alignments is properly, e.g.

# mount -o sunit=4096,swidth=8192 /dev/sdb1 /mnt
# mount | grep sdb1
/dev/sdb1 on /mnt type xfs (rw,sunit=4096,swidth=8192)

However, neither sunit nor swidth is shown from the xfs_info output.

# xfs_info /mnt
meta-data=/dev/sdb1    isize=256    agcount=4, agsize=262144 blks
          =             sectsz=512   attr=2
data     =             bsize=4096   blocks=1048576, imaxpct=25
          =             sunit=0      swidth=0 blks
		       ^^^^^^^^^^^^^^^^^^^^^^^^^^
naming   =version 2    bsize=4096   ascii-ci=0
log      =internal     bsize=4096   blocks=2560, version=2
          =             sectsz=512   sunit=0 blks, lazy-count=1
realtime =none         extsz=4096   blocks=0, rtextents=0

The reason is that the alignment can only be changed if the relevant super block is already configured with alignments, otherwise, the given value will be silently ignored, so it's better to tell user that the alignment-changing can not take affect in one way or another.

With this fix, the attempt to mount a storage without strip alignment setup on super block will failed if XFS_MOUNT_RETERR is enabled, or just ignore the given alignment and drop a warning to indicate the cause in syslog.

# mount -o sunit=4096,swidth=8192 /dev/sdb1 /mnt
mount: wrong fs type, bad option, bad superblock on /dev/sdb1,
        missing codepage or helper program, or other error
        In some cases useful info is found in syslog - try
        dmesg | tail  or so

# dmesg|tail
.......
XFS (sdb1): can not change alignment: no data alignment on superblock

Signed-off-by Jie Liu<[email protected]>

---
  fs/xfs/xfs_mount.c |    7 +++++++
  1 file changed, 7 insertions(+)

fs/xfs/xfs_mount.c +0 -0

index 3806088..bc7fdd4 100644

Quoted 47 lines · Jeff Liu

From: Jie Liu<[email protected]>

As per the mount man page, sunit and swidth can be changed via mount options. For XFS, on the face of it, those options seems works if the specified alignments is properly, e.g.

# mount -o sunit=4096,swidth=8192 /dev/sdb1 /mnt
# mount | grep sdb1
/dev/sdb1 on /mnt type xfs (rw,sunit=4096,swidth=8192)

However, neither sunit nor swidth is shown from the xfs_info output.

# xfs_info /mnt
meta-data=/dev/sdb1    isize=256    agcount=4, agsize=262144 blks
          =             sectsz=512   attr=2
data     =             bsize=4096   blocks=1048576, imaxpct=25
          =             sunit=0      swidth=0 blks
		       ^^^^^^^^^^^^^^^^^^^^^^^^^^
naming   =version 2    bsize=4096   ascii-ci=0
log      =internal     bsize=4096   blocks=2560, version=2
          =             sectsz=512   sunit=0 blks, lazy-count=1
realtime =none         extsz=4096   blocks=0, rtextents=0

The reason is that the alignment can only be changed if the relevant super block is already configured with alignments, otherwise, the given value will be silently ignored, so it's better to tell user that the alignment-changing can not take affect in one way or another.

With this fix, the attempt to mount a storage without strip alignment setup on super block will failed if XFS_MOUNT_RETERR is enabled, or just ignore the given alignment and drop a warning to indicate the cause in syslog.

# mount -o sunit=4096,swidth=8192 /dev/sdb1 /mnt
mount: wrong fs type, bad option, bad superblock on /dev/sdb1,
        missing codepage or helper program, or other error
        In some cases useful info is found in syslog - try
        dmesg | tail  or so

# dmesg|tail
.......
XFS (sdb1): can not change alignment: no data alignment on superblock

Signed-off-by Jie Liu<[email protected]>

---
  fs/xfs/xfs_mount.c |    7 +++++++
  1 file changed, 7 insertions(+)

fs/xfs/xfs_mount.c xfs_update_alignment(xfs_mount_t *mp) · line 929

Quoted 11 lines

fs/xfs/xfs_mount.c +3 -0

index 3806088..bc7fdd4 100644
@@ -924,6 +924,13 @@ xfs_update_alignment(xfs_mount_t *mp)
  				sbp->sb_width = mp->m_swidth;
  				mp->m_update_flags |= XFS_SB_WIDTH;
  			}
+		} else {
+			xfs_warn(mp, "can not change alignment: "
+				"no data alignment on superblock");

suggest that you keep the string together so it can be searched.
"superblock does not support data alignment"

fs/xfs/xfs_mount.c xfs_update_alignment(xfs_mount_t *mp) · line 936

Quoted 8 lines

fs/xfs/xfs_mount.c +4 -0

+			if (mp->m_flags&  XFS_MOUNT_RETERR)
+				return XFS_ERROR(EINVAL);
+			mp->m_dalign = 0;
+			mp->m_swidth = 0;
  		}
  	} else if ((mp->m_flags&  XFS_MOUNT_NOALIGN) != XFS_MOUNT_NOALIGN&&
  		xfs_sb_version_hasdalign(&mp->m_sb)) {

Thanks,

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

From: Jie Liu <[email protected]>

As per the mount man page, sunit and swidth can be changed via mount options. For XFS, on the face of it, those options seems works if the specified alignments is properly, e.g.

# mount -o sunit=4096,swidth=8192 /dev/sdb1 /mnt
# mount | grep sdb1
/dev/sdb1 on /mnt type xfs (rw,sunit=4096,swidth=8192)

However, neither sunit nor swidth is shown from the xfs_info output.

# xfs_info /mnt
meta-data=/dev/sdb1    isize=256    agcount=4, agsize=262144 blks
         =             sectsz=512   attr=2
data     =             bsize=4096   blocks=1048576, imaxpct=25
         =             sunit=0      swidth=0 blks
		       ^^^^^^^^^^^^^^^^^^^^^^^^^^
naming   =version 2    bsize=4096   ascii-ci=0
log      =internal     bsize=4096   blocks=2560, version=2
         =             sectsz=512   sunit=0 blks, lazy-count=1
realtime =none         extsz=4096   blocks=0, rtextents=0

The reason is that the alignment can only be changed if the relevant super block is already configured with alignments, otherwise, the given value will be silently ignored, so it's better to tell user that the alignment-changing can not take affect in one way or another.

With this fix, the attempt to mount a storage without strip alignment setup on super block will failed if XFS_MOUNT_RETERR is enabled, or just ignore the given alignment and drop a warning to indicate the cause in syslog.

# mount -o sunit=4096,swidth=8192 /dev/sdb1 /mnt
mount: wrong fs type, bad option, bad superblock on /dev/sdb1,
       missing codepage or helper program, or other error
       In some cases useful info is found in syslog - try
       dmesg | tail  or so

# dmesg|tail
.......
XFS (sdb1): can not change alignment: no data alignment on superblock

Signed-off-by Jie Liu <[email protected]>

---
 fs/xfs/xfs_mount.c |    7 +++++++
 1 file changed, 7 insertions(+)

fs/xfs/xfs_mount.c +0 -0

index 3806088..bc7fdd4 100644

Quoted 56 lines · Jeff Liu

From: Jie Liu <[email protected]>

As per the mount man page, sunit and swidth can be changed via mount options. For XFS, on the face of it, those options seems works if the specified alignments is properly, e.g.

# mount -o sunit=4096,swidth=8192 /dev/sdb1 /mnt
# mount | grep sdb1
/dev/sdb1 on /mnt type xfs (rw,sunit=4096,swidth=8192)

However, neither sunit nor swidth is shown from the xfs_info output.

# xfs_info /mnt
meta-data=/dev/sdb1    isize=256    agcount=4, agsize=262144 blks
         =             sectsz=512   attr=2
data     =             bsize=4096   blocks=1048576, imaxpct=25
         =             sunit=0      swidth=0 blks
		       ^^^^^^^^^^^^^^^^^^^^^^^^^^
naming   =version 2    bsize=4096   ascii-ci=0
log      =internal     bsize=4096   blocks=2560, version=2
         =             sectsz=512   sunit=0 blks, lazy-count=1
realtime =none         extsz=4096   blocks=0, rtextents=0

The reason is that the alignment can only be changed if the relevant super block is already configured with alignments, otherwise, the given value will be silently ignored, so it's better to tell user that the alignment-changing can not take affect in one way or another.

With this fix, the attempt to mount a storage without strip alignment setup on super block will failed if XFS_MOUNT_RETERR is enabled, or just ignore the given alignment and drop a warning to indicate the cause in syslog.

# mount -o sunit=4096,swidth=8192 /dev/sdb1 /mnt
mount: wrong fs type, bad option, bad superblock on /dev/sdb1,
       missing codepage or helper program, or other error
       In some cases useful info is found in syslog - try
       dmesg | tail  or so

# dmesg|tail
.......
XFS (sdb1): can not change alignment: no data alignment on superblock

Signed-off-by Jie Liu <[email protected]>

---
 fs/xfs/xfs_mount.c |    7 +++++++
 1 file changed, 7 insertions(+)

fs/xfs/xfs_mount.c +1 -0

index 3806088..bc7fdd4 100644
@@ -924,6 +924,13 @@ xfs_update_alignment(xfs_mount_t *mp)
 				sbp->sb_width = mp->m_swidth;
 				mp->m_update_flags |= XFS_SB_WIDTH;
 			}
+		} else {

fs/xfs/xfs_mount.c xfs_update_alignment(xfs_mount_t *mp) · line 933

Quoted 6 lines

fs/xfs/xfs_mount.c +6 -0

+			xfs_warn(mp, "can not change alignment: "
+				"no data alignment on superblock");
+			if (mp->m_flags & XFS_MOUNT_RETERR)
+				return XFS_ERROR(EINVAL);
+			mp->m_dalign = 0;
+			mp->m_swidth = 0;

Can someone tell me why the XFS_MOUNT_RETERR flag exists?

It looks like dead code to me as the only time mp->m_dalign is set prior to calling xfs_update_alignment() is the same code that sets XFS_MOUNT_RETERR in xfs_parseargs().

IOWs, any time we enter this "if (mp->m_dalign)" branch in xfs_update_alignment(), XFS_MOUNT_RETERR is going to be set and so we should always be emitting a warning and returning an error.

If this is correct, Jeff, can you remove the XFS_MOUNT_RETERR flag and get rid of all the dead code in xfs_update_alignment() at the same time, please?

Quoted 14 lines · Dave Chinner

Quoted 13 lines · Jeff Liu

---
 fs/xfs/xfs_mount.c |    7 +++++++
 1 file changed, 7 insertions(+)

fs/xfs/xfs_mount.c +1 -0

index 3806088..bc7fdd4 100644
@@ -924,6 +924,13 @@ xfs_update_alignment(xfs_mount_t *mp)
 				sbp->sb_width = mp->m_swidth;
 				mp->m_update_flags |= XFS_SB_WIDTH;
 			}
+		} else {

Quoted 8 lines

fs/xfs/xfs_mount.c xfs_update_alignment(xfs_mount_t *mp) · line 933

Quoted 6 lines

fs/xfs/xfs_mount.c +6 -0

+			xfs_warn(mp, "can not change alignment: "
+				"no data alignment on superblock");
+			if (mp->m_flags & XFS_MOUNT_RETERR)
+				return XFS_ERROR(EINVAL);
+			mp->m_dalign = 0;
+			mp->m_swidth = 0;

Can someone tell me why the XFS_MOUNT_RETERR flag exists?

This is really a very opportune response because I also worked out another tiny patch for removing XFS_MOUNT_RETERR a few minutes ago, just hesitating if I missed anything or not.

Quoted 7 lines

It looks like dead code to me as the only time mp->m_dalign is set prior to calling xfs_update_alignment() is the same code that sets XFS_MOUNT_RETERR in xfs_parseargs().

IOWs, any time we enter this "if (mp->m_dalign)" branch in xfs_update_alignment(), XFS_MOUNT_RETERR is going to be set and so we should always be emitting a warning and returning an error.

Yes, I realized that as I can not trigger a warning only, it always returning an error to me. :(

Quoted 1 line

Quoted 3 lines

If this is correct, Jeff, can you remove the XFS_MOUNT_RETERR flag and get rid of all the dead code in xfs_update_alignment() at the same time, please?

Sure, I'll post this patch tonight together with another initial patch for fixing transaction space over-reservation we have discussed two weeks ago, xfstests is running now.