| Message ID | 20180614120457.28285-3-hch@lst.de |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/6] fs: factor out a __generic_write_end helper | expand |
On Thu, Jun 14, 2018 at 02:04:53PM +0200, Christoph Hellwig wrote: > We always either ask for a block device or DAX device mapping, so we can > use the same space for both. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/ext2/inode.c | 6 ++++-- > fs/ext4/inode.c | 6 ++++-- > fs/xfs/xfs_iomap.c | 6 ++++-- > include/linux/iomap.h | 6 ++++-- > 4 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c > index 71635909df3b..8aead4e9dbc1 100644 > --- a/fs/ext2/inode.c > +++ b/fs/ext2/inode.c > @@ -815,9 +815,11 @@ static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length, > return ret; > > iomap->flags = 0; > - iomap->bdev = inode->i_sb->s_bdev; > iomap->offset = (u64)first_block << blkbits; > - iomap->dax_dev = sbi->s_daxdev; > + if (IS_DAX(inode)) > + iomap->dax_dev = sbi->s_daxdev; > + else > + iomap->bdev = inode->i_sb->s_bdev; > > if (ret == 0) { > iomap->type = IOMAP_HOLE; > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 2ea07efbe016..79027e99118f 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3524,8 +3524,10 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, > iomap->flags = 0; > if (ext4_inode_datasync_dirty(inode)) > iomap->flags |= IOMAP_F_DIRTY; > - iomap->bdev = inode->i_sb->s_bdev; > - iomap->dax_dev = sbi->s_daxdev; > + if (IS_DAX(inode)) > + iomap->dax_dev = sbi->s_daxdev; > + else > + iomap->bdev = inode->i_sb->s_bdev; > iomap->offset = (u64)first_block << blkbits; > iomap->length = (u64)map.m_len << blkbits; > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c > index c6ce6f9335b6..c9c3d0df5e4c 100644 > --- a/fs/xfs/xfs_iomap.c > +++ b/fs/xfs/xfs_iomap.c > @@ -70,8 +70,10 @@ xfs_bmbt_to_iomap( > } > iomap->offset = XFS_FSB_TO_B(mp, imap->br_startoff); > iomap->length = XFS_FSB_TO_B(mp, imap->br_blockcount); > - iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip)); > - iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip)); > + if (IS_DAX(VFS_I(ip))) > + iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip)); > + else > + iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip)); > } > > xfs_extlen_t > diff --git a/include/linux/iomap.h b/include/linux/iomap.h > index a044a824da85..212f4d59bcbf 100644 > --- a/include/linux/iomap.h > +++ b/include/linux/iomap.h > @@ -53,8 +53,10 @@ struct iomap { > u64 length; /* length of mapping, bytes */ > u16 type; /* type of mapping */ > u16 flags; /* flags for mapping */ > - struct block_device *bdev; /* block device for I/O */ > - struct dax_device *dax_dev; /* dax_dev for dax operations */ > + union { > + struct block_device *bdev; > + struct dax_device *dax_dev; Is this going to blow up iomap_dax_zero? It seems to use both bdev and dax_dev on __dax_zero_page_range, which definitely uses both. (Or did all that get rearranged when I wasn't looking?) Also, I guess this will break iomap swapfiles since it checks iomap->bdev which we stop supplying with this patch... though I have no idea if DAX swapfiles are even supported. What's the harm in supplying both pointers? --D > + }; > }; > > /* > -- > 2.17.1 > > -- > 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 Mon, Jun 18, 2018 at 11:25:57PM -0700, Darrick J. Wong wrote: > Is this going to blow up iomap_dax_zero? It seems to use both bdev and > dax_dev on __dax_zero_page_range, which definitely uses both. > > (Or did all that get rearranged when I wasn't looking?) Ouch, it does. And that looks pretty broken. > Also, I guess this will break iomap swapfiles since it checks > iomap->bdev which we stop supplying with this patch... > though I have no idea if DAX swapfiles are even supported. Not sure if we support it. We didn't use to support it when swap used ->bmap, so until someone volunteers to test it we should disable it with the iomap swapfile code as well. But even then doing a detour through the block layer and thus the bdev makes very little sense. > > What's the harm in supplying both pointers? Just blowing up the size of the iomap. Especially once we add the inline data as the third option.
On Tue, Jun 19, 2018 at 08:44:51AM +0200, Christoph Hellwig wrote: > On Mon, Jun 18, 2018 at 11:25:57PM -0700, Darrick J. Wong wrote: > > Is this going to blow up iomap_dax_zero? It seems to use both bdev and > > dax_dev on __dax_zero_page_range, which definitely uses both. > > > > (Or did all that get rearranged when I wasn't looking?) > > Ouch, it does. And that looks pretty broken. Indeed. > > Also, I guess this will break iomap swapfiles since it checks > > iomap->bdev which we stop supplying with this patch... > > though I have no idea if DAX swapfiles are even supported. > > Not sure if we support it. We didn't use to support it when > swap used ->bmap, so until someone volunteers to test it we > should disable it with the iomap swapfile code as well. But > even then doing a detour through the block layer and thus > the bdev makes very little sense. For now all we do with it is compare iomap->bdev to the swapfile bdev so it'll effectively disable DAX swapfiles in a weird and scary way... ...but maybe we should ask around if anyone cares about DAX swapfiles. Everyone may just run away, but at least we will have tried. :) > > > > What's the harm in supplying both pointers? > > Just blowing up the size of the iomap. Especially once we add > the inline data as the third option. <nod> --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
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c index 71635909df3b..8aead4e9dbc1 100644 --- a/fs/ext2/inode.c +++ b/fs/ext2/inode.c @@ -815,9 +815,11 @@ static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length, return ret; iomap->flags = 0; - iomap->bdev = inode->i_sb->s_bdev; iomap->offset = (u64)first_block << blkbits; - iomap->dax_dev = sbi->s_daxdev; + if (IS_DAX(inode)) + iomap->dax_dev = sbi->s_daxdev; + else + iomap->bdev = inode->i_sb->s_bdev; if (ret == 0) { iomap->type = IOMAP_HOLE; diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 2ea07efbe016..79027e99118f 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3524,8 +3524,10 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, iomap->flags = 0; if (ext4_inode_datasync_dirty(inode)) iomap->flags |= IOMAP_F_DIRTY; - iomap->bdev = inode->i_sb->s_bdev; - iomap->dax_dev = sbi->s_daxdev; + if (IS_DAX(inode)) + iomap->dax_dev = sbi->s_daxdev; + else + iomap->bdev = inode->i_sb->s_bdev; iomap->offset = (u64)first_block << blkbits; iomap->length = (u64)map.m_len << blkbits; diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c index c6ce6f9335b6..c9c3d0df5e4c 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -70,8 +70,10 @@ xfs_bmbt_to_iomap( } iomap->offset = XFS_FSB_TO_B(mp, imap->br_startoff); iomap->length = XFS_FSB_TO_B(mp, imap->br_blockcount); - iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip)); - iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip)); + if (IS_DAX(VFS_I(ip))) + iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip)); + else + iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip)); } xfs_extlen_t diff --git a/include/linux/iomap.h b/include/linux/iomap.h index a044a824da85..212f4d59bcbf 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -53,8 +53,10 @@ struct iomap { u64 length; /* length of mapping, bytes */ u16 type; /* type of mapping */ u16 flags; /* flags for mapping */ - struct block_device *bdev; /* block device for I/O */ - struct dax_device *dax_dev; /* dax_dev for dax operations */ + union { + struct block_device *bdev; + struct dax_device *dax_dev; + }; }; /*
We always either ask for a block device or DAX device mapping, so we can use the same space for both. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/ext2/inode.c | 6 ++++-- fs/ext4/inode.c | 6 ++++-- fs/xfs/xfs_iomap.c | 6 ++++-- include/linux/iomap.h | 6 ++++-- 4 files changed, 16 insertions(+), 8 deletions(-)