| Message ID | 9faf09627cfa469437b76edb73ac7cc822dc33c8.1526488995.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Wed, May 16, 2018 at 09:45:50AM -0700, Omar Sandoval wrote: > + if ((iomap->type != IOMAP_MAPPED && iomap->type != IOMAP_UNWRITTEN) || > + iomap->addr == IOMAP_NULL_ADDR) { > + pr_err("swapon: file has unallocated extents\n"); > + return -EINVAL; > + } The two are really different cases - IOMAP_NULL_ADDR shouldn't really happen for any of the above. Although we might have to move the inline check before the type check above for the message to make sense. I have a patch in the local queue that makes inline a type instead of a flag, btw as it really isn't a flag.
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; } /*