* [PATCH v2 0/2] iomap: swapfile tweaks @ 2018-05-16 16:45 Omar Sandoval 2018-05-16 16:45 ` [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files Omar Sandoval 2018-05-16 16:45 ` [PATCH v2 2/2] iomap: don't allow holes in swapfiles Omar Sandoval 0 siblings, 2 replies; 9+ messages in thread From: Omar Sandoval @ 2018-05-16 16:45 UTC (permalink / raw) To: Darrick J . Wong, linux-xfs Cc: linux-fsdevel, Jan Kara, Christoph Hellwig, Aleksei Besogonov, kernel-team From: Omar Sandoval <osandov@fb.com> Changes from v1: - Added patch 1 to provide more useful error messages (suggested by me) and move the bdev check (suggested by Christoph) - Changed patch 2 to simply delete the hole check, since it will be caught by the mapped || unwritten check below Based on xfs-linux/for-next. Thanks! Omar Sandoval (2): iomap: provide more useful errors for invalid swap files iomap: don't allow holes in swapfiles fs/iomap.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) -- 2.17.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files 2018-05-16 16:45 [PATCH v2 0/2] iomap: swapfile tweaks Omar Sandoval @ 2018-05-16 16:45 ` Omar Sandoval 2018-05-16 16:54 ` Darrick J. Wong 2018-05-16 17:25 ` Christoph Hellwig 2018-05-16 16:45 ` [PATCH v2 2/2] iomap: don't allow holes in swapfiles Omar Sandoval 1 sibling, 2 replies; 9+ messages in thread From: Omar Sandoval @ 2018-05-16 16:45 UTC (permalink / raw) To: Darrick J . Wong, linux-xfs Cc: linux-fsdevel, Jan Kara, Christoph Hellwig, Aleksei Besogonov, kernel-team 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> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files 2018-05-16 16:45 ` [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files Omar Sandoval @ 2018-05-16 16:54 ` Darrick J. Wong 2018-05-16 17:25 ` Christoph Hellwig 1 sibling, 0 replies; 9+ messages in thread From: Darrick J. Wong @ 2018-05-16 16:54 UTC (permalink / raw) To: Omar Sandoval Cc: linux-xfs, linux-fsdevel, Jan Kara, Christoph Hellwig, Aleksei Besogonov, kernel-team 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files 2018-05-16 16:45 ` [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files Omar Sandoval 2018-05-16 16:54 ` Darrick J. Wong @ 2018-05-16 17:25 ` Christoph Hellwig 2018-05-16 17:32 ` Omar Sandoval 1 sibling, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2018-05-16 17:25 UTC (permalink / raw) To: Omar Sandoval Cc: Darrick J . Wong, linux-xfs, linux-fsdevel, Jan Kara, Christoph Hellwig, Aleksei Besogonov, kernel-team 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files 2018-05-16 17:25 ` Christoph Hellwig @ 2018-05-16 17:32 ` Omar Sandoval 2018-05-16 17:46 ` Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Omar Sandoval @ 2018-05-16 17:32 UTC (permalink / raw) To: Christoph Hellwig Cc: Darrick J . Wong, linux-xfs, linux-fsdevel, Jan Kara, Aleksei Besogonov, kernel-team On Wed, May 16, 2018 at 10:25:31AM -0700, Christoph Hellwig wrote: > 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. 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. diff --git a/fs/iomap.c b/fs/iomap.c index d193390a1c20..89517442e296 100644 --- a/fs/iomap.c +++ b/fs/iomap.c @@ -1214,26 +1214,37 @@ static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos, struct iomap_swapfile_info *isi = data; int error; + /* No inline data. */ + if (iomap->flags & IOMAP_F_DATA_INLINE) { + pr_err("swapon: file is inline\n"); + return -EINVAL; + } + /* Skip holes. */ 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) { + 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; + /* No uncommitted metadata or shared blocks. */ + 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; + } - /* 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 +1261,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; } /* ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files 2018-05-16 17:32 ` Omar Sandoval @ 2018-05-16 17:46 ` Christoph Hellwig 2018-05-16 18:11 ` Darrick J. Wong 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2018-05-16 17:46 UTC (permalink / raw) To: Omar Sandoval Cc: Christoph Hellwig, Darrick J . Wong, linux-xfs, linux-fsdevel, Jan Kara, Aleksei Besogonov, kernel-team On Wed, May 16, 2018 at 10:32:51AM -0700, Omar Sandoval wrote: > On Wed, May 16, 2018 at 10:25:31AM -0700, Christoph Hellwig wrote: > > 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. > > 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! ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files 2018-05-16 17:46 ` Christoph Hellwig @ 2018-05-16 18:11 ` Darrick J. Wong 2018-05-16 18:22 ` Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Darrick J. Wong @ 2018-05-16 18:11 UTC (permalink / raw) To: Christoph Hellwig Cc: Omar Sandoval, linux-xfs, linux-fsdevel, Jan Kara, Aleksei Besogonov, kernel-team On Wed, May 16, 2018 at 10:46:45AM -0700, Christoph Hellwig wrote: > On Wed, May 16, 2018 at 10:32:51AM -0700, Omar Sandoval wrote: > > On Wed, May 16, 2018 at 10:25:31AM -0700, Christoph Hellwig wrote: > > > 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. 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. > > 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! Yeah, the v3 patch looks ok. --D > -- > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files 2018-05-16 18:11 ` Darrick J. Wong @ 2018-05-16 18:22 ` Christoph Hellwig 0 siblings, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2018-05-16 18:22 UTC (permalink / raw) To: Darrick J. Wong Cc: Christoph Hellwig, Omar Sandoval, linux-xfs, linux-fsdevel, Jan Kara, Aleksei Besogonov, kernel-team On Wed, May 16, 2018 at 11:11:42AM -0700, Darrick J. Wong wrote: > 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, patches welcome :) > particularly for things like IOMAP_F_BOUNDARY that > don't have any meaning in xfs. Yikes, how did that even end in the tree? That whole boundary thing already made little sene on buffer heads, and even less so in the generic iomap code. I think we just need to allocate a few flags for fs specific uses and move it into gfs2. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] iomap: don't allow holes in swapfiles 2018-05-16 16:45 [PATCH v2 0/2] iomap: swapfile tweaks Omar Sandoval 2018-05-16 16:45 ` [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files Omar Sandoval @ 2018-05-16 16:45 ` Omar Sandoval 1 sibling, 0 replies; 9+ messages in thread From: Omar Sandoval @ 2018-05-16 16:45 UTC (permalink / raw) To: Darrick J . Wong, linux-xfs Cc: linux-fsdevel, Jan Kara, Christoph Hellwig, Aleksei Besogonov, kernel-team From: Omar Sandoval <osandov@fb.com> generic_swapfile_activate() doesn't allow holes, so we should be consistent here. This is also a bit safer: if the user creates a swapfile with, say, truncate -s $SIZE followed by mkswap, they should really get an error and not much less swap space than they expected. swapon(8) will error out before calling swapon(2) if the file has holes, anyways. Fixes: 9d93388b0afe ("iomap: add a swapfile activation function") Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> Signed-off-by: Omar Sandoval <osandov@fb.com> --- fs/iomap.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/fs/iomap.c b/fs/iomap.c index 318724375ffe..5a1099d935c3 100644 --- a/fs/iomap.c +++ b/fs/iomap.c @@ -1214,10 +1214,6 @@ static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos, struct iomap_swapfile_info *isi = data; int error; - /* Skip holes. */ - if (iomap->type == IOMAP_HOLE) - goto out; - /* Only real or unwritten extents. */ if ((iomap->type != IOMAP_MAPPED && iomap->type != IOMAP_UNWRITTEN) || iomap->addr == IOMAP_NULL_ADDR) { @@ -1258,7 +1254,6 @@ static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos, return error; memcpy(&isi->iomap, iomap, sizeof(isi->iomap)); } -out: return count; } -- 2.17.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-05-16 18:22 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-16 16:45 [PATCH v2 0/2] iomap: swapfile tweaks Omar Sandoval 2018-05-16 16:45 ` [PATCH v2 1/2] iomap: provide more useful errors for invalid swap files Omar Sandoval 2018-05-16 16:54 ` Darrick J. Wong 2018-05-16 17:25 ` Christoph Hellwig 2018-05-16 17:32 ` Omar Sandoval 2018-05-16 17:46 ` Christoph Hellwig 2018-05-16 18:11 ` Darrick J. Wong 2018-05-16 18:22 ` Christoph Hellwig 2018-05-16 16:45 ` [PATCH v2 2/2] iomap: don't allow holes in swapfiles Omar Sandoval
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).