Re: [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, May 16, 2018 at 09:45:50AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Currently, for an invalid swap file, we print the same error message
> regardless of the reason. This isn't very useful for an admin, who will
> likely want to know why exactly they can't use their swap file. So,
> let's add specific error messages for each reason, and also move the
> bdev check after the flags checks, since the latter are more
> fundamental.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/iomap.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/iomap.c b/fs/iomap.c
> index d193390a1c20..318724375ffe 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -1218,22 +1218,32 @@ static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos,
>  	if (iomap->type == IOMAP_HOLE)
>  		goto out;
>  
> -	/* Only one bdev per swap file. */
> -	if (iomap->bdev != isi->sis->bdev)
> -		goto err;
> -
>  	/* Only real or unwritten extents. */
> -	if (iomap->type != IOMAP_MAPPED && iomap->type != IOMAP_UNWRITTEN)
> -		goto err;
> +	if ((iomap->type != IOMAP_MAPPED && iomap->type != IOMAP_UNWRITTEN) ||
> +	    iomap->addr == IOMAP_NULL_ADDR) {
> +		pr_err("swapon: file has unallocated extents\n");
> +		return -EINVAL;
> +	}
>  
>  	/* No uncommitted metadata or shared blocks or inline data. */
> -	if (iomap->flags & (IOMAP_F_DIRTY | IOMAP_F_SHARED |
> -			    IOMAP_F_DATA_INLINE))
> -		goto err;
> +	if (iomap->flags & IOMAP_F_DIRTY) {
> +		pr_err("swapon: file is not committed\n");
> +		return -EINVAL;
> +	}
> +	if (iomap->flags & IOMAP_F_SHARED) {
> +		pr_err("swapon: file has shared extents\n");
> +		return -EINVAL;
> +	}
> +	if (iomap->flags & IOMAP_F_DATA_INLINE) {
> +		pr_err("swapon: file is inline\n");
> +		return -EINVAL;
> +	}
>  
> -	/* No null physical addresses. */
> -	if (iomap->addr == IOMAP_NULL_ADDR)
> -		goto err;
> +	/* Only one bdev per swap file. */
> +	if (iomap->bdev != isi->sis->bdev) {
> +		pr_err("swapon: file is on multiple devices\n");
> +		return -EINVAL;
> +	}
>  
>  	if (isi->iomap.length == 0) {
>  		/* No accumulated extent, so just store it. */
> @@ -1250,9 +1260,6 @@ static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos,
>  	}
>  out:
>  	return count;
> -err:
> -	pr_err("swapon: file cannot be used for swap\n");
> -	return -EINVAL;
>  }
>  
>  /*
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html