[PATCH] iomap: don't override sis->bdev in xfs_iomap_swapfile_activate

[PATCH] iomap: don't override sis->bdev in xfs_iomap_swapfile_activate

[Christoph Hellwig]

The swapon code itself sets sis->bdev up early, and performs various check
on the block devices.  Changing it later in the fact thus will cause a
mismatch of capabilities and must be avoided.  The practical implication
of this change is that it forbids swapping to the RT subvolume, which might
have had all kinds of issues anyway.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_aops.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 58e937be24ce..f9929a952ef1 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -637,7 +637,6 @@ xfs_iomap_swapfile_activate(
 	struct file			*swap_file,
 	sector_t			*span)
 {
-	sis->bdev = xfs_inode_buftarg(XFS_I(file_inode(swap_file)))->bt_bdev;
 	return iomap_swapfile_activate(sis, swap_file, span,
 			&xfs_read_iomap_ops);
 }
-- 
2.24.1

Re: [PATCH] iomap: don't override sis->bdev in xfs_iomap_swapfile_activate

[Darrick J. Wong]

On Sun, Mar 01, 2020 at 07:49:25AM -0700, Christoph Hellwig wrote:
> The swapon code itself sets sis->bdev up early, and performs various check
> on the block devices.  Changing it later in the fact thus will cause a
> mismatch of capabilities and must be avoided.

What kind of mismatch?  Are you talking about the bdi_cap_* and
blk_queue_nonrot() logic in swapon()?  I wonder how much of that could
be moved to after the ->swapfile_activate call.

> The practical implication
> of this change is that it forbids swapping to the RT subvolume, which might
> have had all kinds of issues anyway.

<shrug> I didn't find any the one time I tried it on a pair of
homogeneous devices. :)

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_aops.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 58e937be24ce..f9929a952ef1 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -637,7 +637,6 @@ xfs_iomap_swapfile_activate(
>  	struct file			*swap_file,
>  	sector_t			*span)
>  {
> -	sis->bdev = xfs_inode_buftarg(XFS_I(file_inode(swap_file)))->bt_bdev;

That said, btrfs copypasta'd this when they ported to iomap swapfile, so
that needs fixing too.

--D

>  	return iomap_swapfile_activate(sis, swap_file, span,
>  			&xfs_read_iomap_ops);
>  }
> -- 
> 2.24.1
> 

Re: [PATCH] iomap: don't override sis->bdev in xfs_iomap_swapfile_activate

[Christoph Hellwig]

On Tue, Mar 03, 2020 at 08:51:57AM -0800, Darrick J. Wong wrote:
> On Sun, Mar 01, 2020 at 07:49:25AM -0700, Christoph Hellwig wrote:
> > The swapon code itself sets sis->bdev up early, and performs various check
> > on the block devices.  Changing it later in the fact thus will cause a
> > mismatch of capabilities and must be avoided.
> 
> What kind of mismatch?  Are you talking about the bdi_cap_* and
> blk_queue_nonrot() logic in swapon()?  I wonder how much of that could
> be moved to after the ->swapfile_activate call.

The thing I ran into is the zone check with my zoned XFS prototype
code.  But when you look at the nonrot checks that will cause
resource leaks due to the override, and thus is the main is the
main issue for now.

I suspect much of this could be cleaned up one way or another, but
the layering of this code is horrible, so it would be a bigger
job.

btrfs hasn't picked up the iomap changes yet, but the next resend
should drop the bdev assignment as well.