iomap: swapfile tweaks

From: Omar Sandoval <[email protected]>

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 <[email protected]>

---
 fs/iomap.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

fs/iomap.c +22 -15

index d193390a1c20..318724375ffe 100644
@@ -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;
 }
 
 /*

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Review

Quoted 5 lines · Omar Sandoval

(quoted diff) +5 -0

+	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.

Quoted 11 lines · Christoph Hellwig

fs/iomap.c static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos, · line 1231

Quoted 5 lines · Omar Sandoval

fs/iomap.c +5 -0

+	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.

Quoted 2 lines

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.

So something like this, moving the inline check and removing the hole check since that doesn't make sense for mapped or unwritten? Then the inline flag check can be converted to a type check.

Quoted 12 lines · Omar Sandoval

Quoted 11 lines · Christoph Hellwig

fs/iomap.c static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos, · line 1231

Quoted 5 lines · Omar Sandoval

fs/iomap.c +5 -0

+	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.

Quoted 2 lines

Quoted 2 lines

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.

Quoted 3 lines

So something like this, moving the inline check and removing the hole check since that doesn't make sense for mapped or unwritten? Then the inline flag check can be converted to a type check.

Yes, this looks great!

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

That's what I thought -- we only see NULL_ADDR for holes and delalloc extents, and we already check for both of those. But I didn't see anything in iomap.h expressly saying that, so I decided to be defensive and check it anyway.

IOWs it would be helpful to have a little more documentation about which flags go together,

particularly for things like IOMAP_F_BOUNDARY that don't have any meaning in xfs.