[5/7] xfs: remove the iosizelog variable in xfs_parseargs
diff mbox series

Message ID 20191025174026.31878-6-hch@lst.de
State New
Headers show
Series
  • [1/7] xfs: cleanup stat blksize reporting
Related show

Commit Message

Christoph Hellwig Oct. 25, 2019, 5:40 p.m. UTC
There is no real need for a local variables here - either the I/O size
is valid and gets applied to the mount structure, or it is invalid and
the mount will fail entirely.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_super.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Comments

Eric Sandeen Oct. 25, 2019, 6:35 p.m. UTC | #1
On 10/25/19 12:40 PM, Christoph Hellwig wrote:
> There is no real need for a local variables here - either the I/O size
> is valid and gets applied to the mount structure, or it is invalid and
> the mount will fail entirely.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_super.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index ee2dde897fb7..1467f4bebc41 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -161,7 +161,6 @@ xfs_parseargs(
>  	char			*p;
>  	substring_t		args[MAX_OPT_ARGS];
>  	int			iosize = 0;
> -	uint8_t			iosizelog = 0;
>  
>  	/*
>  	 * set up the mount name first so all the errors will refer to the
> @@ -229,7 +228,8 @@ xfs_parseargs(
>  		case Opt_biosize:
>  			if (suffix_kstrtoint(args, 10, &iosize))
>  				return -EINVAL;
> -			iosizelog = ffs(iosize) - 1;
> +			mp->m_writeio_log = ffs(iosize) - 1;
> +			mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
>  			break;
>  		case Opt_grpid:
>  		case Opt_bsdgroups:
> @@ -397,17 +397,14 @@ xfs_parseargs(
>  		return -EINVAL;
>  	}
>  
> -	if (iosizelog) {
> -		if (iosizelog > XFS_MAX_IO_LOG ||
> -		    iosizelog < XFS_MIN_IO_LOG) {
> +	if (mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) {
> +		if (mp->m_writeio_log > XFS_MAX_IO_LOG ||
> +		    mp->m_writeio_log < XFS_MIN_IO_LOG) {

There's a slight change in behavior here.

Before, "mount -o biosize=0" would pass, because iosizelog, though specified,
did not satisfy "if (iosizelog)"and so it was never tested, so it was
essentially ignored.

Now the same specification will fail mount.

To make this a completely cosmetic change, perhaps this should test
mp->m_writeio_log rather than the flag.

If a change in behavior is desired I think that should be an explicit
2nd change.

>  			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
> -				iosizelog, XFS_MIN_IO_LOG,
> -				XFS_MAX_IO_LOG);
> +				mp->m_writeio_log,
> +				XFS_MIN_IO_LOG, XFS_MAX_IO_LOG);
>  			return -EINVAL;
>  		}
> -
> -		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> -		mp->m_writeio_log = iosizelog;
>  	}
>  
>  	return 0;
>
Eric Sandeen Oct. 25, 2019, 7:03 p.m. UTC | #2
On 10/25/19 1:35 PM, Eric Sandeen wrote:

> There's a slight change in behavior here.
> 
> Before, "mount -o biosize=0" would pass, because iosizelog, though specified,
> did not satisfy "if (iosizelog)"and so it was never tested, so it was
> essentially ignored.

sorry make that "-o allocsize=0"

Which I guess /is/ documented as being invalid, we just never rejected it
before.

Still a change in behavior and worth being explicit about I think.

-Eric
Christoph Hellwig Oct. 26, 2019, 5:47 a.m. UTC | #3
On Fri, Oct 25, 2019 at 02:03:06PM -0500, Eric Sandeen wrote:
> sorry make that "-o allocsize=0"
> 
> Which I guess /is/ documented as being invalid, we just never rejected it
> before.
> 
> Still a change in behavior and worth being explicit about I think.

I'll updated the changelog to mention that.

Patch
diff mbox series

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index ee2dde897fb7..1467f4bebc41 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -161,7 +161,6 @@  xfs_parseargs(
 	char			*p;
 	substring_t		args[MAX_OPT_ARGS];
 	int			iosize = 0;
-	uint8_t			iosizelog = 0;
 
 	/*
 	 * set up the mount name first so all the errors will refer to the
@@ -229,7 +228,8 @@  xfs_parseargs(
 		case Opt_biosize:
 			if (suffix_kstrtoint(args, 10, &iosize))
 				return -EINVAL;
-			iosizelog = ffs(iosize) - 1;
+			mp->m_writeio_log = ffs(iosize) - 1;
+			mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
 			break;
 		case Opt_grpid:
 		case Opt_bsdgroups:
@@ -397,17 +397,14 @@  xfs_parseargs(
 		return -EINVAL;
 	}
 
-	if (iosizelog) {
-		if (iosizelog > XFS_MAX_IO_LOG ||
-		    iosizelog < XFS_MIN_IO_LOG) {
+	if (mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) {
+		if (mp->m_writeio_log > XFS_MAX_IO_LOG ||
+		    mp->m_writeio_log < XFS_MIN_IO_LOG) {
 			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
-				iosizelog, XFS_MIN_IO_LOG,
-				XFS_MAX_IO_LOG);
+				mp->m_writeio_log,
+				XFS_MIN_IO_LOG, XFS_MAX_IO_LOG);
 			return -EINVAL;
 		}
-
-		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
-		mp->m_writeio_log = iosizelog;
 	}
 
 	return 0;