| Message ID | e722c29f7291b9c31533fd8b2e84d7f469c3ac88.1526452889.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, May 15, 2018 at 11:45:38PM -0700, Omar Sandoval wrote: > 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") > Signed-off-by: Omar Sandoval <osandov@fb.com> > --- > Hey, Darrick, I noticed this while writing up a generic xfstest to test > that the Btrfs swap support patches don't allow a swapfile with holes. > It'd be nice if we were all consistent :) This is based on > xfs-linux/for-next. Feel free to fold it in to your patch or apply it > separately as you see fit. Thanks! I sent a testcase of my own ("generic: test swapfile creation, activation, and deactivation") a while back; would you mind sending out yours so we can combine them into a single testcase? > fs/iomap.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/iomap.c b/fs/iomap.c > index d193390a1c20..ba559adaa327 100644 > --- a/fs/iomap.c > +++ b/fs/iomap.c > @@ -1214,9 +1214,9 @@ static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos, > struct iomap_swapfile_info *isi = data; > int error; > > - /* Skip holes. */ > + /* No holes. */ > if (iomap->type == IOMAP_HOLE) > - goto out; > + goto err; Ok. I agree that it looks weird to mount a swap file with holes, so I guess the least-surprise principle applies here and we should emulate the old behavior completely. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > > /* Only one bdev per swap file. */ > if (iomap->bdev != isi->sis->bdev) > -- > 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 -- 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 08:50:00AM -0700, Darrick J. Wong wrote: > > Hey, Darrick, I noticed this while writing up a generic xfstest to test > > that the Btrfs swap support patches don't allow a swapfile with holes. > > It'd be nice if we were all consistent :) This is based on > > xfs-linux/for-next. Feel free to fold it in to your patch or apply it > > separately as you see fit. Thanks! > > I sent a testcase of my own ("generic: test swapfile creation, > activation, and deactivation") a while back; would you mind sending out > yours so we can combine them into a single testcase? Wasn't the desire to support holes the rationale for the Aleksei version of the iomap swapfile patch? -- 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 08:51:46AM -0700, Christoph Hellwig wrote: > On Wed, May 16, 2018 at 08:50:00AM -0700, Darrick J. Wong wrote: > > > Hey, Darrick, I noticed this while writing up a generic xfstest to test > > > that the Btrfs swap support patches don't allow a swapfile with holes. > > > It'd be nice if we were all consistent :) This is based on > > > xfs-linux/for-next. Feel free to fold it in to your patch or apply it > > > separately as you see fit. Thanks! > > > > I sent a testcase of my own ("generic: test swapfile creation, > > activation, and deactivation") a while back; would you mind sending out > > yours so we can combine them into a single testcase? > > Wasn't the desire to support holes the rationale for the Aleksei > version of the iomap swapfile patch? Ah, so it was. FWIW I'm not sure why you'd /want/ a holey swapfile? TBH my motivation for iomap swapfile is purely to remove bmap callers. :) --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 -- 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 08:56:38AM -0700, Darrick J. Wong wrote: > On Wed, May 16, 2018 at 08:51:46AM -0700, Christoph Hellwig wrote: > > On Wed, May 16, 2018 at 08:50:00AM -0700, Darrick J. Wong wrote: > > > > Hey, Darrick, I noticed this while writing up a generic xfstest to test > > > > that the Btrfs swap support patches don't allow a swapfile with holes. > > > > It'd be nice if we were all consistent :) This is based on > > > > xfs-linux/for-next. Feel free to fold it in to your patch or apply it > > > > separately as you see fit. Thanks! > > > > > > I sent a testcase of my own ("generic: test swapfile creation, > > > activation, and deactivation") a while back; would you mind sending out > > > yours so we can combine them into a single testcase? Sure thing, I have a small pile of tests. I'm still working on some Btrfs-specific ones, but I can send out the generic ones and we can figure out how to merge them. > > Wasn't the desire to support holes the rationale for the Aleksei > > version of the iomap swapfile patch? > > Ah, so it was. FWIW I'm not sure why you'd /want/ a holey swapfile? From reading the old thread, it looks like Aleksei just wanted fallocated swap files to work: "I've traced the problem to bmap(), used in generic_swapfile_activate call, which returns 0 for blocks inside holes created by fallocate". Are holes in that sense are different from actual holes in the iomap sense? -- 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
> > - /* Skip holes. */ > + /* No holes. */ > if (iomap->type == IOMAP_HOLE) > - goto out; > + goto err; If we end up not allowing holes this check can just be removed entirely. We already check for the allow types below. I have to admit I would move that type check above the bdev check, though. -- 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:19:55AM -0700, Omar Sandoval wrote: > > > Wasn't the desire to support holes the rationale for the Aleksei > > > version of the iomap swapfile patch? > > > > Ah, so it was. FWIW I'm not sure why you'd /want/ a holey swapfile? > > >From reading the old thread, it looks like Aleksei just wanted > fallocated swap files to work: "I've traced the problem to bmap(), used > in generic_swapfile_activate call, which returns 0 for blocks inside > holes created by fallocate". Oh, that makes more sense. > Are holes in that sense are different from > actual holes in the iomap sense? Unwritten extents aren't actually holes in any sense, so they are very different and should work with the iomap swapfile code. -- 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:23:29AM -0700, Christoph Hellwig wrote: > > > > - /* Skip holes. */ > > + /* No holes. */ > > if (iomap->type == IOMAP_HOLE) > > - goto out; > > + goto err; > > If we end up not allowing holes this check can just be removed > entirely. We already check for the allow types below. I have to > admit I would move that type check above the bdev check, though. True, although I wonder if we should be logging more specific error messages instead of a generic "not a valid swap file" message. At least for my Btrfs patches, that's what I do. I'll send a v2 with a second patch doing that. -- 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
diff --git a/fs/iomap.c b/fs/iomap.c index d193390a1c20..ba559adaa327 100644 --- a/fs/iomap.c +++ b/fs/iomap.c @@ -1214,9 +1214,9 @@ static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos, struct iomap_swapfile_info *isi = data; int error; - /* Skip holes. */ + /* No holes. */ if (iomap->type == IOMAP_HOLE) - goto out; + goto err; /* Only one bdev per swap file. */ if (iomap->bdev != isi->sis->bdev)