| Message ID | 1454829553-29499-2-git-send-email-ross.zwisler@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Feb 6, 2016 at 11:19 PM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > dax_clear_blocks() needs a valid struct block_device and previously it was > using inode->i_sb->s_bdev in all cases. This is correct for normal inodes > on mounted ext2, ext4 and XFS filesystems, but is incorrect for DAX raw > block devices and for XFS real-time devices. > > Instead, have the caller pass in a struct block_device pointer which it > knows to be correct. > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > --- > fs/dax.c | 4 ++-- > fs/ext2/inode.c | 5 +++-- > fs/xfs/xfs_aops.c | 2 +- > fs/xfs/xfs_aops.h | 1 + > fs/xfs/xfs_bmap_util.c | 4 +++- > include/linux/dax.h | 3 ++- > 6 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index 227974a..4592241 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -83,9 +83,9 @@ struct page *read_dax_sector(struct block_device *bdev, sector_t n) > * and hence this means the stack from this point must follow GFP_NOFS > * semantics for all operations. > */ > -int dax_clear_blocks(struct inode *inode, sector_t block, long _size) > +int dax_clear_blocks(struct inode *inode, struct block_device *bdev, > + sector_t block, long _size) Since this is a bdev relative routine we should also resolve the sector, i.e. the signature should drop the inode: int dax_clear_sectors(struct block_device *bdev, sector_t sector, long _size)
On Sun, Feb 07, 2016 at 12:19:12AM -0700, Ross Zwisler wrote: > dax_clear_blocks() needs a valid struct block_device and previously it was > using inode->i_sb->s_bdev in all cases. This is correct for normal inodes > on mounted ext2, ext4 and XFS filesystems, but is incorrect for DAX raw > block devices and for XFS real-time devices. > > Instead, have the caller pass in a struct block_device pointer which it > knows to be correct. .... > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > index 07ef29b..f722ba2 100644 > --- a/fs/xfs/xfs_bmap_util.c > +++ b/fs/xfs/xfs_bmap_util.c > @@ -73,9 +73,11 @@ xfs_zero_extent( > xfs_daddr_t sector = xfs_fsb_to_db(ip, start_fsb); > sector_t block = XFS_BB_TO_FSBT(mp, sector); > ssize_t size = XFS_FSB_TO_B(mp, count_fsb); > + struct inode *inode = VFS_I(ip); > > if (IS_DAX(VFS_I(ip))) > - return dax_clear_blocks(VFS_I(ip), block, size); > + return dax_clear_blocks(inode, xfs_find_bdev_for_inode(inode), > + block, size); Get rid of the local inode variable and use VFS_I(ip) like the code originally did. Do not change code that is unrelated to the modifcation being made, especially when it results in making the code an inconsistent mess of mixed pointer constructs.... Cheers, Dave.
On Mon, Feb 08, 2016 at 09:03:29AM +1100, Dave Chinner wrote: > On Sun, Feb 07, 2016 at 12:19:12AM -0700, Ross Zwisler wrote: > > dax_clear_blocks() needs a valid struct block_device and previously it was > > using inode->i_sb->s_bdev in all cases. This is correct for normal inodes > > on mounted ext2, ext4 and XFS filesystems, but is incorrect for DAX raw > > block devices and for XFS real-time devices. > > > > Instead, have the caller pass in a struct block_device pointer which it > > knows to be correct. > .... > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > > index 07ef29b..f722ba2 100644 > > --- a/fs/xfs/xfs_bmap_util.c > > +++ b/fs/xfs/xfs_bmap_util.c > > @@ -73,9 +73,11 @@ xfs_zero_extent( > > xfs_daddr_t sector = xfs_fsb_to_db(ip, start_fsb); > > sector_t block = XFS_BB_TO_FSBT(mp, sector); > > ssize_t size = XFS_FSB_TO_B(mp, count_fsb); > > + struct inode *inode = VFS_I(ip); > > > > if (IS_DAX(VFS_I(ip))) > > - return dax_clear_blocks(VFS_I(ip), block, size); > > + return dax_clear_blocks(inode, xfs_find_bdev_for_inode(inode), > > + block, size); > > Get rid of the local inode variable and use VFS_I(ip) like the code > originally did. Do not change code that is unrelated to the > modifcation being made, especially when it results in making > the code an inconsistent mess of mixed pointer constructs.... The local 'inode' variable was added to avoid multiple calls for VFS_I() for the same 'ip'. That said, I'm happy to make the change.
On Sun, Feb 07, 2016 at 10:19:29AM -0800, Dan Williams wrote: > On Sat, Feb 6, 2016 at 11:19 PM, Ross Zwisler > <ross.zwisler@linux.intel.com> wrote: > > dax_clear_blocks() needs a valid struct block_device and previously it was > > using inode->i_sb->s_bdev in all cases. This is correct for normal inodes > > on mounted ext2, ext4 and XFS filesystems, but is incorrect for DAX raw > > block devices and for XFS real-time devices. > > > > Instead, have the caller pass in a struct block_device pointer which it > > knows to be correct. > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > --- > > fs/dax.c | 4 ++-- > > fs/ext2/inode.c | 5 +++-- > > fs/xfs/xfs_aops.c | 2 +- > > fs/xfs/xfs_aops.h | 1 + > > fs/xfs/xfs_bmap_util.c | 4 +++- > > include/linux/dax.h | 3 ++- > > 6 files changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/fs/dax.c b/fs/dax.c > > index 227974a..4592241 100644 > > --- a/fs/dax.c > > +++ b/fs/dax.c > > @@ -83,9 +83,9 @@ struct page *read_dax_sector(struct block_device *bdev, sector_t n) > > * and hence this means the stack from this point must follow GFP_NOFS > > * semantics for all operations. > > */ > > -int dax_clear_blocks(struct inode *inode, sector_t block, long _size) > > +int dax_clear_blocks(struct inode *inode, struct block_device *bdev, > > + sector_t block, long _size) > > Since this is a bdev relative routine we should also resolve the > sector, i.e. the signature should drop the inode: > > int dax_clear_sectors(struct block_device *bdev, sector_t sector, long _size) The inode is still needed because dax_clear_blocks() needs inode->i_blkbits. Unless there is some easy way to get this from the bdev that I'm not seeing?
> On Feb 7, 2016, at 6:46 PM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > >> On Sun, Feb 07, 2016 at 10:19:29AM -0800, Dan Williams wrote: >> On Sat, Feb 6, 2016 at 11:19 PM, Ross Zwisler >> <ross.zwisler@linux.intel.com> wrote: >>> dax_clear_blocks() needs a valid struct block_device and previously it was >>> using inode->i_sb->s_bdev in all cases. This is correct for normal inodes >>> on mounted ext2, ext4 and XFS filesystems, but is incorrect for DAX raw >>> block devices and for XFS real-time devices. >>> >>> Instead, have the caller pass in a struct block_device pointer which it >>> knows to be correct. >>> >>> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> >>> --- >>> fs/dax.c | 4 ++-- >>> fs/ext2/inode.c | 5 +++-- >>> fs/xfs/xfs_aops.c | 2 +- >>> fs/xfs/xfs_aops.h | 1 + >>> fs/xfs/xfs_bmap_util.c | 4 +++- >>> include/linux/dax.h | 3 ++- >>> 6 files changed, 12 insertions(+), 7 deletions(-) >>> >>> diff --git a/fs/dax.c b/fs/dax.c >>> index 227974a..4592241 100644 >>> --- a/fs/dax.c >>> +++ b/fs/dax.c >>> @@ -83,9 +83,9 @@ struct page *read_dax_sector(struct block_device *bdev, sector_t n) >>> * and hence this means the stack from this point must follow GFP_NOFS >>> * semantics for all operations. >>> */ >>> -int dax_clear_blocks(struct inode *inode, sector_t block, long _size) >>> +int dax_clear_blocks(struct inode *inode, struct block_device *bdev, >>> + sector_t block, long _size) >> >> Since this is a bdev relative routine we should also resolve the >> sector, i.e. the signature should drop the inode: >> >> int dax_clear_sectors(struct block_device *bdev, sector_t sector, long _size) > > The inode is still needed because dax_clear_blocks() needs inode->i_blkbits. > Unless there is some easy way to get this from the bdev that I'm not seeing? Never mind, you are passing in the sector, not the block. Sure, this seems better - I'll fix this for v2.
On Sun, Feb 07, 2016 at 06:44:09PM -0700, Ross Zwisler wrote: > On Mon, Feb 08, 2016 at 09:03:29AM +1100, Dave Chinner wrote: > > On Sun, Feb 07, 2016 at 12:19:12AM -0700, Ross Zwisler wrote: > > > dax_clear_blocks() needs a valid struct block_device and previously it was > > > using inode->i_sb->s_bdev in all cases. This is correct for normal inodes > > > on mounted ext2, ext4 and XFS filesystems, but is incorrect for DAX raw > > > block devices and for XFS real-time devices. > > > > > > Instead, have the caller pass in a struct block_device pointer which it > > > knows to be correct. > > .... > > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > > > index 07ef29b..f722ba2 100644 > > > --- a/fs/xfs/xfs_bmap_util.c > > > +++ b/fs/xfs/xfs_bmap_util.c > > > @@ -73,9 +73,11 @@ xfs_zero_extent( > > > xfs_daddr_t sector = xfs_fsb_to_db(ip, start_fsb); > > > sector_t block = XFS_BB_TO_FSBT(mp, sector); > > > ssize_t size = XFS_FSB_TO_B(mp, count_fsb); > > > + struct inode *inode = VFS_I(ip); > > > > > > if (IS_DAX(VFS_I(ip))) > > > - return dax_clear_blocks(VFS_I(ip), block, size); > > > + return dax_clear_blocks(inode, xfs_find_bdev_for_inode(inode), > > > + block, size); > > > > Get rid of the local inode variable and use VFS_I(ip) like the code > > originally did. Do not change code that is unrelated to the > > modifcation being made, especially when it results in making > > the code an inconsistent mess of mixed pointer constructs.... > > The local 'inode' variable was added to avoid multiple calls for VFS_I() for > the same 'ip'. My point is you didn't achieve that. The end result of your patch is: struct inode *inode = VFS_I(ip); if (IS_DAX(VFS_I(ip))) return dax_clear_blocks(inode, xfs_find_bdev_for_inode(inode), block, size); So now we have a local variable, but we still have 2 calls to VFS_I(ip). i.e. this makes the code harder to read and understand than before for no benefit. Cheers, Dave.
On Mon, Feb 08, 2016 at 04:17:25PM +1100, Dave Chinner wrote: > On Sun, Feb 07, 2016 at 06:44:09PM -0700, Ross Zwisler wrote: > > On Mon, Feb 08, 2016 at 09:03:29AM +1100, Dave Chinner wrote: > > > On Sun, Feb 07, 2016 at 12:19:12AM -0700, Ross Zwisler wrote: > > > > dax_clear_blocks() needs a valid struct block_device and previously it was > > > > using inode->i_sb->s_bdev in all cases. This is correct for normal inodes > > > > on mounted ext2, ext4 and XFS filesystems, but is incorrect for DAX raw > > > > block devices and for XFS real-time devices. > > > > > > > > Instead, have the caller pass in a struct block_device pointer which it > > > > knows to be correct. > > > .... > > > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > > > > index 07ef29b..f722ba2 100644 > > > > --- a/fs/xfs/xfs_bmap_util.c > > > > +++ b/fs/xfs/xfs_bmap_util.c > > > > @@ -73,9 +73,11 @@ xfs_zero_extent( > > > > xfs_daddr_t sector = xfs_fsb_to_db(ip, start_fsb); > > > > sector_t block = XFS_BB_TO_FSBT(mp, sector); > > > > ssize_t size = XFS_FSB_TO_B(mp, count_fsb); > > > > + struct inode *inode = VFS_I(ip); > > > > > > > > if (IS_DAX(VFS_I(ip))) > > > > - return dax_clear_blocks(VFS_I(ip), block, size); > > > > + return dax_clear_blocks(inode, xfs_find_bdev_for_inode(inode), > > > > + block, size); > > > > > > Get rid of the local inode variable and use VFS_I(ip) like the code > > > originally did. Do not change code that is unrelated to the > > > modifcation being made, especially when it results in making > > > the code an inconsistent mess of mixed pointer constructs.... > > > > The local 'inode' variable was added to avoid multiple calls for VFS_I() for > > the same 'ip'. > > My point is you didn't achieve that. The end result of your patch > is: > > struct inode *inode = VFS_I(ip); > > if (IS_DAX(VFS_I(ip))) > return dax_clear_blocks(inode, xfs_find_bdev_for_inode(inode), > block, size); > > So now we have a local variable, but we still have 2 calls to > VFS_I(ip). i.e. this makes the code harder to read and understand > than before for no benefit. *facepalm* Yep, thanks for the correction.
diff --git a/fs/dax.c b/fs/dax.c index 227974a..4592241 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -83,9 +83,9 @@ struct page *read_dax_sector(struct block_device *bdev, sector_t n) * and hence this means the stack from this point must follow GFP_NOFS * semantics for all operations. */ -int dax_clear_blocks(struct inode *inode, sector_t block, long _size) +int dax_clear_blocks(struct inode *inode, struct block_device *bdev, + sector_t block, long _size) { - struct block_device *bdev = inode->i_sb->s_bdev; struct blk_dax_ctl dax = { .sector = block << (inode->i_blkbits - 9), .size = _size, diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c index 338eefd..277a32b 100644 --- a/fs/ext2/inode.c +++ b/fs/ext2/inode.c @@ -737,8 +737,9 @@ static int ext2_get_blocks(struct inode *inode, * so that it's not found by another thread before it's * initialised */ - err = dax_clear_blocks(inode, le32_to_cpu(chain[depth-1].key), - 1 << inode->i_blkbits); + err = dax_clear_blocks(inode, inode->i_sb->s_bdev, + le32_to_cpu(chain[depth-1].key), + 1 << inode->i_blkbits); if (err) { mutex_unlock(&ei->truncate_mutex); goto cleanup; diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index 379c089..fc20518 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -55,7 +55,7 @@ xfs_count_page_state( } while ((bh = bh->b_this_page) != head); } -STATIC struct block_device * +struct block_device * xfs_find_bdev_for_inode( struct inode *inode) { diff --git a/fs/xfs/xfs_aops.h b/fs/xfs/xfs_aops.h index f6ffc9a..a4343c6 100644 --- a/fs/xfs/xfs_aops.h +++ b/fs/xfs/xfs_aops.h @@ -62,5 +62,6 @@ int xfs_get_blocks_dax_fault(struct inode *inode, sector_t offset, struct buffer_head *map_bh, int create); extern void xfs_count_page_state(struct page *, int *, int *); +extern struct block_device *xfs_find_bdev_for_inode(struct inode *); #endif /* __XFS_AOPS_H__ */ diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index 07ef29b..f722ba2 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -73,9 +73,11 @@ xfs_zero_extent( xfs_daddr_t sector = xfs_fsb_to_db(ip, start_fsb); sector_t block = XFS_BB_TO_FSBT(mp, sector); ssize_t size = XFS_FSB_TO_B(mp, count_fsb); + struct inode *inode = VFS_I(ip); if (IS_DAX(VFS_I(ip))) - return dax_clear_blocks(VFS_I(ip), block, size); + return dax_clear_blocks(inode, xfs_find_bdev_for_inode(inode), + block, size); /* * let the block layer decide on the fastest method of diff --git a/include/linux/dax.h b/include/linux/dax.h index 8204c3d..bad27b0 100644 --- a/include/linux/dax.h +++ b/include/linux/dax.h @@ -7,7 +7,8 @@ ssize_t dax_do_io(struct kiocb *, struct inode *, struct iov_iter *, loff_t, get_block_t, dio_iodone_t, int flags); -int dax_clear_blocks(struct inode *, sector_t block, long size); +int dax_clear_blocks(struct inode *inode, struct block_device *bdev, + sector_t block, long _size); int dax_zero_page_range(struct inode *, loff_t from, unsigned len, get_block_t); int dax_truncate_page(struct inode *, loff_t from, get_block_t); int dax_fault(struct vm_area_struct *, struct vm_fault *, get_block_t,
dax_clear_blocks() needs a valid struct block_device and previously it was using inode->i_sb->s_bdev in all cases. This is correct for normal inodes on mounted ext2, ext4 and XFS filesystems, but is incorrect for DAX raw block devices and for XFS real-time devices. Instead, have the caller pass in a struct block_device pointer which it knows to be correct. Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> --- fs/dax.c | 4 ++-- fs/ext2/inode.c | 5 +++-- fs/xfs/xfs_aops.c | 2 +- fs/xfs/xfs_aops.h | 1 + fs/xfs/xfs_bmap_util.c | 4 +++- include/linux/dax.h | 3 ++- 6 files changed, 12 insertions(+), 7 deletions(-)