* [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal
@ 2019-07-31 14:12 Carlos Maiolino
2019-07-31 14:12 ` [PATCH 1/9] fs: Enable bmap() function to properly return errors Carlos Maiolino
` (9 more replies)
0 siblings, 10 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-07-31 14:12 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
Hi.
This is the 4th version of the complete series with the goal to deprecate and
eventually remove ->bmap() interface, in lieu of FIEMAP.
Besides the rebase of the patchset against latest Linus's tree, the only changes
in this patchset regarding to the previous version, are concentrated in patches
4/9 and 8/9.
In patch 4 (fibmap: Use bmap instead of ->bmap method in ioctl_fibmap), the
difference compared with previous version, is a change in ioclt_fibmap() return
value, I spotted while testing this new version with filesystems using data
inlined in inodes. It now returns 0 in case of error instead an error value,
otherwise it would be an user interface change.
In patch 8 (Use FIEMAP for FIBMAP calls), there are minor changes regarding V3.
It just contains a coding style fix pointed by Andreas in the previous version,
but now, it also include changes to all filesystems which supports both FIEMAP
and FIBMAP, and need some sort of special handling (like inlined data, reflinked
inodes, etc).
Again, Patch 9 is xfs-specific removal of ->bmap() interface, without any
changes compared to the previous version.
I do apologize for taking so long to rework this patchset, I've got busy with
other stuff.
Comments are appreciated, specially regarding if the error values returned by
ioctl_fibmap() make sense.
Cheers
Carlos Maiolino (9):
fs: Enable bmap() function to properly return errors
cachefiles: drop direct usage of ->bmap method.
ecryptfs: drop direct calls to ->bmap
fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
fs: Move start and length fiemap fields into fiemap_extent_info
iomap: Remove length and start fields from iomap_fiemap
fiemap: Use a callback to fill fiemap extents
Use FIEMAP for FIBMAP calls
xfs: Get rid of ->bmap
drivers/md/md-bitmap.c | 16 ++++--
fs/bad_inode.c | 3 +-
fs/btrfs/inode.c | 5 +-
fs/cachefiles/rdwr.c | 27 ++++-----
fs/ecryptfs/mmap.c | 16 ++----
fs/ext2/ext2.h | 3 +-
fs/ext2/inode.c | 6 +-
fs/ext4/ext4.h | 3 +-
fs/ext4/extents.c | 15 +++--
fs/f2fs/data.c | 15 ++++-
fs/f2fs/f2fs.h | 3 +-
fs/gfs2/inode.c | 9 ++-
fs/hpfs/file.c | 4 +-
fs/inode.c | 105 +++++++++++++++++++++++++++++----
fs/ioctl.c | 128 ++++++++++++++++++++++++++---------------
fs/iomap.c | 40 ++-----------
fs/jbd2/journal.c | 22 ++++---
fs/nilfs2/inode.c | 5 +-
fs/nilfs2/nilfs.h | 3 +-
fs/ocfs2/extent_map.c | 13 ++++-
fs/ocfs2/extent_map.h | 3 +-
fs/overlayfs/inode.c | 5 +-
fs/xfs/xfs_aops.c | 24 --------
fs/xfs/xfs_iops.c | 19 +++---
fs/xfs/xfs_trace.h | 1 -
include/linux/fs.h | 33 +++++++----
include/linux/iomap.h | 2 +-
mm/page_io.c | 11 ++--
28 files changed, 320 insertions(+), 219 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 1/9] fs: Enable bmap() function to properly return errors
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
@ 2019-07-31 14:12 ` Carlos Maiolino
2019-07-31 14:12 ` [PATCH 2/9] cachefiles: drop direct usage of ->bmap method Carlos Maiolino
` (8 subsequent siblings)
9 siblings, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-07-31 14:12 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
By now, bmap() will either return the physical block number related to
the requested file offset or 0 in case of error or the requested offset
maps into a hole.
This patch makes the needed changes to enable bmap() to proper return
errors, using the return value as an error return, and now, a pointer
must be passed to bmap() to be filled with the mapped physical block.
It will change the behavior of bmap() on return:
- negative value in case of error
- zero on success or map fell into a hole
In case of a hole, the *block will be zero too
Since this is a prep patch, by now, the only error return is -EINVAL if
->bmap doesn't exist.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
drivers/md/md-bitmap.c | 16 ++++++++++------
fs/inode.c | 30 +++++++++++++++++-------------
fs/jbd2/journal.c | 22 +++++++++++++++-------
include/linux/fs.h | 2 +-
mm/page_io.c | 11 +++++++----
5 files changed, 50 insertions(+), 31 deletions(-)
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index 1cd4f991792c..0668b2dd290e 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -363,7 +363,7 @@ static int read_page(struct file *file, unsigned long index,
int ret = 0;
struct inode *inode = file_inode(file);
struct buffer_head *bh;
- sector_t block;
+ sector_t block, blk_cur;
pr_debug("read bitmap file (%dB @ %llu)\n", (int)PAGE_SIZE,
(unsigned long long)index << PAGE_SHIFT);
@@ -374,17 +374,21 @@ static int read_page(struct file *file, unsigned long index,
goto out;
}
attach_page_buffers(page, bh);
- block = index << (PAGE_SHIFT - inode->i_blkbits);
+ blk_cur = index << (PAGE_SHIFT - inode->i_blkbits);
while (bh) {
+ block = blk_cur;
+
if (count == 0)
bh->b_blocknr = 0;
else {
- bh->b_blocknr = bmap(inode, block);
- if (bh->b_blocknr == 0) {
- /* Cannot use this file! */
+ ret = bmap(inode, &block);
+ if (ret || !block) {
ret = -EINVAL;
+ bh->b_blocknr = 0;
goto out;
}
+
+ bh->b_blocknr = block;
bh->b_bdev = inode->i_sb->s_bdev;
if (count < (1<<inode->i_blkbits))
count = 0;
@@ -398,7 +402,7 @@ static int read_page(struct file *file, unsigned long index,
set_buffer_mapped(bh);
submit_bh(REQ_OP_READ, 0, bh);
}
- block++;
+ blk_cur++;
bh = bh->b_this_page;
}
page->index = index;
diff --git a/fs/inode.c b/fs/inode.c
index e9d97add2b36..824fa54d393d 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1577,21 +1577,25 @@ EXPORT_SYMBOL(iput);
/**
* bmap - find a block number in a file
- * @inode: inode of file
- * @block: block to find
- *
- * Returns the block number on the device holding the inode that
- * is the disk block number for the block of the file requested.
- * That is, asked for block 4 of inode 1 the function will return the
- * disk block relative to the disk start that holds that block of the
- * file.
+ * @inode: inode owning the block number being requested
+ * @*block: pointer containing the block to find
+ *
+ * Replaces the value in *block with the block number on the device holding
+ * corresponding to the requested block number in the file.
+ * That is, asked for block 4 of inode 1 the function will replace the
+ * 4 in *block, with disk block relative to the disk start that holds that
+ * block of the file.
+ *
+ * Returns -EINVAL in case of error, 0 otherwise. If mapping falls into a
+ * hole, returns 0 and *block is also set to 0.
*/
-sector_t bmap(struct inode *inode, sector_t block)
+int bmap(struct inode *inode, sector_t *block)
{
- sector_t res = 0;
- if (inode->i_mapping->a_ops->bmap)
- res = inode->i_mapping->a_ops->bmap(inode->i_mapping, block);
- return res;
+ if (!inode->i_mapping->a_ops->bmap)
+ return -EINVAL;
+
+ *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
+ return 0;
}
EXPORT_SYMBOL(bmap);
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 382c030cc78b..b5b3b7bfa8f6 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -798,18 +798,23 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
{
int err = 0;
unsigned long long ret;
+ sector_t block = 0;
if (journal->j_inode) {
- ret = bmap(journal->j_inode, blocknr);
- if (ret)
- *retp = ret;
- else {
+ block = blocknr;
+ ret = bmap(journal->j_inode, &block);
+
+ if (ret || !block) {
printk(KERN_ALERT "%s: journal block not found "
"at offset %lu on %s\n",
__func__, blocknr, journal->j_devname);
err = -EIO;
__journal_abort_soft(journal, err);
+
+ } else {
+ *retp = block;
}
+
} else {
*retp = blocknr; /* +journal->j_blk_offset */
}
@@ -1235,11 +1240,14 @@ journal_t *jbd2_journal_init_dev(struct block_device *bdev,
journal_t *jbd2_journal_init_inode(struct inode *inode)
{
journal_t *journal;
+ sector_t blocknr;
char *p;
- unsigned long long blocknr;
+ int err = 0;
+
+ blocknr = 0;
+ err = bmap(inode, &blocknr);
- blocknr = bmap(inode, 0);
- if (!blocknr) {
+ if (err || !blocknr) {
pr_err("%s: Cannot locate journal superblock\n",
__func__);
return NULL;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8b42df09b04c..d5e7c744aea6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2803,7 +2803,7 @@ static inline ssize_t generic_write_sync(struct kiocb *iocb, ssize_t count)
extern void emergency_sync(void);
extern void emergency_remount(void);
#ifdef CONFIG_BLOCK
-extern sector_t bmap(struct inode *, sector_t);
+extern int bmap(struct inode *, sector_t *);
#endif
extern int notify_change(struct dentry *, struct iattr *, struct inode **);
extern int inode_permission(struct inode *, int);
diff --git a/mm/page_io.c b/mm/page_io.c
index 2e8019d0e048..dc878c47916c 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -177,8 +177,9 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
cond_resched();
- first_block = bmap(inode, probe_block);
- if (first_block == 0)
+ first_block = probe_block;
+ ret = bmap(inode, &first_block);
+ if (ret || !first_block)
goto bad_bmap;
/*
@@ -193,9 +194,11 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
block_in_page++) {
sector_t block;
- block = bmap(inode, probe_block + block_in_page);
- if (block == 0)
+ block = probe_block + block_in_page;
+ ret = bmap(inode, &block);
+ if (ret || !block)
goto bad_bmap;
+
if (block != first_block + block_in_page) {
/* Discontiguity */
probe_block++;
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 2/9] cachefiles: drop direct usage of ->bmap method.
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
2019-07-31 14:12 ` [PATCH 1/9] fs: Enable bmap() function to properly return errors Carlos Maiolino
@ 2019-07-31 14:12 ` Carlos Maiolino
2019-07-31 14:12 ` [PATCH 3/9] ecryptfs: drop direct calls to ->bmap Carlos Maiolino
` (7 subsequent siblings)
9 siblings, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-07-31 14:12 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
Replace the direct usage of ->bmap method by a bmap() call.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
fs/cachefiles/rdwr.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c
index 8a577409d030..1a74aa978056 100644
--- a/fs/cachefiles/rdwr.c
+++ b/fs/cachefiles/rdwr.c
@@ -400,7 +400,7 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
struct cachefiles_object *object;
struct cachefiles_cache *cache;
struct inode *inode;
- sector_t block0, block;
+ sector_t block;
unsigned shift;
int ret;
@@ -416,7 +416,6 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
inode = d_backing_inode(object->backer);
ASSERT(S_ISREG(inode->i_mode));
- ASSERT(inode->i_mapping->a_ops->bmap);
ASSERT(inode->i_mapping->a_ops->readpages);
/* calculate the shift required to use bmap */
@@ -432,12 +431,14 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
* enough for this as it doesn't indicate errors, but it's all we've
* got for the moment
*/
- block0 = page->index;
- block0 <<= shift;
+ block = page->index;
+ block <<= shift;
+
+ ret = bmap(inode, &block);
+ ASSERT(!ret);
- block = inode->i_mapping->a_ops->bmap(inode->i_mapping, block0);
_debug("%llx -> %llx",
- (unsigned long long) block0,
+ (unsigned long long) (page->index << shift),
(unsigned long long) block);
if (block) {
@@ -715,7 +716,6 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op,
inode = d_backing_inode(object->backer);
ASSERT(S_ISREG(inode->i_mode));
- ASSERT(inode->i_mapping->a_ops->bmap);
ASSERT(inode->i_mapping->a_ops->readpages);
/* calculate the shift required to use bmap */
@@ -732,7 +732,7 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op,
ret = space ? -ENODATA : -ENOBUFS;
list_for_each_entry_safe(page, _n, pages, lru) {
- sector_t block0, block;
+ sector_t block;
/* we assume the absence or presence of the first block is a
* good enough indication for the page as a whole
@@ -740,13 +740,14 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op,
* good enough for this as it doesn't indicate errors, but
* it's all we've got for the moment
*/
- block0 = page->index;
- block0 <<= shift;
+ block = page->index;
+ block <<= shift;
+
+ ret = bmap(inode, &block);
+ ASSERT(!ret);
- block = inode->i_mapping->a_ops->bmap(inode->i_mapping,
- block0);
_debug("%llx -> %llx",
- (unsigned long long) block0,
+ (unsigned long long) (page->index << shift),
(unsigned long long) block);
if (block) {
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 3/9] ecryptfs: drop direct calls to ->bmap
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
2019-07-31 14:12 ` [PATCH 1/9] fs: Enable bmap() function to properly return errors Carlos Maiolino
2019-07-31 14:12 ` [PATCH 2/9] cachefiles: drop direct usage of ->bmap method Carlos Maiolino
@ 2019-07-31 14:12 ` Carlos Maiolino
2019-07-31 14:12 ` [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap Carlos Maiolino
` (6 subsequent siblings)
9 siblings, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-07-31 14:12 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
Replace direct ->bmap calls by bmap() method.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
fs/ecryptfs/mmap.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index cdf358b209d9..ff323dccef36 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -538,16 +538,12 @@ static int ecryptfs_write_end(struct file *file,
static sector_t ecryptfs_bmap(struct address_space *mapping, sector_t block)
{
- int rc = 0;
- struct inode *inode;
- struct inode *lower_inode;
-
- inode = (struct inode *)mapping->host;
- lower_inode = ecryptfs_inode_to_lower(inode);
- if (lower_inode->i_mapping->a_ops->bmap)
- rc = lower_inode->i_mapping->a_ops->bmap(lower_inode->i_mapping,
- block);
- return rc;
+ struct inode *lower_inode = ecryptfs_inode_to_lower(mapping->host);
+ int ret = bmap(lower_inode, &block);
+
+ if (ret)
+ return 0;
+ return block;
}
const struct address_space_operations ecryptfs_aops = {
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
` (2 preceding siblings ...)
2019-07-31 14:12 ` [PATCH 3/9] ecryptfs: drop direct calls to ->bmap Carlos Maiolino
@ 2019-07-31 14:12 ` Carlos Maiolino
2019-07-31 23:12 ` Darrick J. Wong
2019-07-31 14:12 ` [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info Carlos Maiolino
` (5 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-07-31 14:12 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
Now we have the possibility of proper error return in bmap, use bmap()
function in ioctl_fibmap() instead of calling ->bmap method directly.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
Changelog:
V4:
- Ensure ioctl_fibmap() returns 0 in case of error returned from
bmap(). Otherwise we'll be changing the user interface (which
returns 0 in case of error)
V3:
- Rename usr_blk to ur_block
V2:
- Use a local sector_t variable to asign the block number
instead of using direct casting.
fs/ioctl.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index fef3a6bf7c78..6b589c873bc2 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -53,19 +53,28 @@ EXPORT_SYMBOL(vfs_ioctl);
static int ioctl_fibmap(struct file *filp, int __user *p)
{
- struct address_space *mapping = filp->f_mapping;
- int res, block;
+ struct inode *inode = file_inode(filp);
+ int error, ur_block;
+ sector_t block;
- /* do we support this mess? */
- if (!mapping->a_ops->bmap)
- return -EINVAL;
if (!capable(CAP_SYS_RAWIO))
return -EPERM;
- res = get_user(block, p);
- if (res)
- return res;
- res = mapping->a_ops->bmap(mapping, block);
- return put_user(res, p);
+
+ error = get_user(ur_block, p);
+ if (error)
+ return error;
+
+ block = ur_block;
+ error = bmap(inode, &block);
+
+ if (error)
+ ur_block = 0;
+ else
+ ur_block = block;
+
+ error = put_user(ur_block, p);
+
+ return error;
}
/**
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
` (3 preceding siblings ...)
2019-07-31 14:12 ` [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap Carlos Maiolino
@ 2019-07-31 14:12 ` Carlos Maiolino
2019-07-31 23:28 ` Darrick J. Wong
2019-07-31 14:12 ` [PATCH 6/9] iomap: Remove length and start fields from iomap_fiemap Carlos Maiolino
` (4 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-07-31 14:12 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
As the overall goal to deprecate fibmap, Christoph suggested a rework of
the ->fiemap API, in a way we could pass to it a callback to fill the
fiemap structure (one of these callbacks being fiemap_fill_next_extent).
To avoid the need to add several fields into the ->fiemap method, aggregate
everything into a single data structure, and pass it along.
This patch isn't suppose to add any functional change, only to update
filesystems providing ->fiemap() method.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
fs/bad_inode.c | 3 +--
fs/btrfs/inode.c | 5 +++--
fs/ext2/ext2.h | 3 +--
fs/ext2/inode.c | 6 ++----
fs/ext4/ext4.h | 3 +--
fs/ext4/extents.c | 8 ++++----
fs/f2fs/data.c | 5 +++--
fs/f2fs/f2fs.h | 3 +--
fs/gfs2/inode.c | 5 +++--
fs/hpfs/file.c | 4 ++--
fs/ioctl.c | 16 ++++++++++------
fs/nilfs2/inode.c | 5 +++--
fs/nilfs2/nilfs.h | 3 +--
fs/ocfs2/extent_map.c | 5 +++--
fs/ocfs2/extent_map.h | 3 +--
fs/overlayfs/inode.c | 5 ++---
fs/xfs/xfs_iops.c | 10 +++++-----
include/linux/fs.h | 21 +++++++++++----------
18 files changed, 57 insertions(+), 56 deletions(-)
diff --git a/fs/bad_inode.c b/fs/bad_inode.c
index 8035d2a44561..21dfaf876814 100644
--- a/fs/bad_inode.c
+++ b/fs/bad_inode.c
@@ -120,8 +120,7 @@ static struct posix_acl *bad_inode_get_acl(struct inode *inode, int type)
}
static int bad_inode_fiemap(struct inode *inode,
- struct fiemap_extent_info *fieinfo, u64 start,
- u64 len)
+ struct fiemap_extent_info *fieinfo)
{
return -EIO;
}
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 82fdda8ff5ab..caa06a8ac767 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8600,9 +8600,10 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
#define BTRFS_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
-static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- __u64 start, __u64 len)
+static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
+ u64 start = fieinfo->fi_start;
+ u64 len = fieinfo->fi_len;
int ret;
ret = fiemap_check_flags(fieinfo, BTRFS_FIEMAP_FLAGS);
diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 10ab238de9a6..284df1af9474 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -760,8 +760,7 @@ extern int ext2_get_block(struct inode *, sector_t, struct buffer_head *, int);
extern int ext2_setattr (struct dentry *, struct iattr *);
extern int ext2_getattr (const struct path *, struct kstat *, u32, unsigned int);
extern void ext2_set_inode_flags(struct inode *inode);
-extern int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- u64 start, u64 len);
+extern int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo);
/* ioctl.c */
extern long ext2_ioctl(struct file *, unsigned int, unsigned long);
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index c27c27300d95..267392335f38 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -855,11 +855,9 @@ const struct iomap_ops ext2_iomap_ops = {
const struct iomap_ops ext2_iomap_ops;
#endif /* CONFIG_FS_DAX */
-int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- u64 start, u64 len)
+int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
- return generic_block_fiemap(inode, fieinfo, start, len,
- ext2_get_block);
+ return generic_block_fiemap(inode, fieinfo, ext2_get_block);
}
static int ext2_writepage(struct page *page, struct writeback_control *wbc)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 82ffdacdc7fa..e4cb40b5893b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3154,8 +3154,7 @@ extern struct ext4_ext_path *ext4_find_extent(struct inode *, ext4_lblk_t,
extern void ext4_ext_drop_refs(struct ext4_ext_path *);
extern int ext4_ext_check_inode(struct inode *inode);
extern ext4_lblk_t ext4_ext_next_allocated_block(struct ext4_ext_path *path);
-extern int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- __u64 start, __u64 len);
+extern int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo);
extern int ext4_ext_precache(struct inode *inode);
extern int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len);
extern int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 0f89f5190cd7..436e564ebdd6 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5038,9 +5038,10 @@ static int ext4_xattr_fiemap(struct inode *inode,
return (error < 0 ? error : 0);
}
-int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- __u64 start, __u64 len)
+int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
+ u64 start = fieinfo->fi_start;
+ u64 len = fieinfo->fi_len;
ext4_lblk_t start_blk;
int error = 0;
@@ -5062,8 +5063,7 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
/* fallback to generic here if not in extents fmt */
if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
- return generic_block_fiemap(inode, fieinfo, start, len,
- ext4_get_block);
+ return generic_block_fiemap(inode, fieinfo, ext4_get_block);
if (fiemap_check_flags(fieinfo, EXT4_FIEMAP_FLAGS))
return -EBADR;
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 9727944139f2..2979ca40d192 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1409,9 +1409,10 @@ static int f2fs_xattr_fiemap(struct inode *inode,
return (err < 0 ? err : 0);
}
-int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- u64 start, u64 len)
+int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
+ u64 start = fieinfo->fi_start;
+ u64 len = fieinfo->fi_len;
struct buffer_head map_bh;
sector_t start_blk, last_blk;
pgoff_t next_pgofs;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 87f75ebd2fd6..fb33809c2552 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3155,8 +3155,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio);
void __do_map_lock(struct f2fs_sb_info *sbi, int flag, bool lock);
int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
int create, int flag);
-int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- u64 start, u64 len);
+int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo);
bool f2fs_should_update_inplace(struct inode *inode, struct f2fs_io_info *fio);
bool f2fs_should_update_outplace(struct inode *inode, struct f2fs_io_info *fio);
void f2fs_invalidate_page(struct page *page, unsigned int offset,
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 998051c4aea7..5e84d5963506 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -2004,9 +2004,10 @@ static int gfs2_getattr(const struct path *path, struct kstat *stat,
return 0;
}
-static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- u64 start, u64 len)
+static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
+ u64 start = fieinfo->fi_start;
+ u64 len = fieinfo->fi_len;
struct gfs2_inode *ip = GFS2_I(inode);
struct gfs2_holder gh;
int ret;
diff --git a/fs/hpfs/file.c b/fs/hpfs/file.c
index 1ecec124e76f..0eece4ae1f11 100644
--- a/fs/hpfs/file.c
+++ b/fs/hpfs/file.c
@@ -190,9 +190,9 @@ static sector_t _hpfs_bmap(struct address_space *mapping, sector_t block)
return generic_block_bmap(mapping, block, hpfs_get_block);
}
-static int hpfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, u64 start, u64 len)
+static int hpfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
- return generic_block_fiemap(inode, fieinfo, start, len, hpfs_get_block);
+ return generic_block_fiemap(inode, fieinfo, hpfs_get_block);
}
const struct address_space_operations hpfs_aops = {
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 6b589c873bc2..ad8edcb10dc9 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -210,6 +210,8 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
fieinfo.fi_flags = fiemap.fm_flags;
fieinfo.fi_extents_max = fiemap.fm_extent_count;
fieinfo.fi_extents_start = ufiemap->fm_extents;
+ fieinfo.fi_start = fiemap.fm_start;
+ fieinfo.fi_len = len;
if (fiemap.fm_extent_count != 0 &&
!access_ok(fieinfo.fi_extents_start,
@@ -219,7 +221,7 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
filemap_write_and_wait(inode->i_mapping);
- error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start, len);
+ error = inode->i_op->fiemap(inode, &fieinfo);
fiemap.fm_flags = fieinfo.fi_flags;
fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
@@ -296,9 +298,11 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
*/
int __generic_block_fiemap(struct inode *inode,
- struct fiemap_extent_info *fieinfo, loff_t start,
- loff_t len, get_block_t *get_block)
+ struct fiemap_extent_info *fieinfo,
+ get_block_t *get_block)
{
+ loff_t start = fieinfo->fi_start;
+ loff_t len = fieinfo->fi_len;
struct buffer_head map_bh;
sector_t start_blk, last_blk;
loff_t isize = i_size_read(inode);
@@ -455,12 +459,12 @@ EXPORT_SYMBOL(__generic_block_fiemap);
*/
int generic_block_fiemap(struct inode *inode,
- struct fiemap_extent_info *fieinfo, u64 start,
- u64 len, get_block_t *get_block)
+ struct fiemap_extent_info *fieinfo,
+ get_block_t *get_block)
{
int ret;
inode_lock(inode);
- ret = __generic_block_fiemap(inode, fieinfo, start, len, get_block);
+ ret = __generic_block_fiemap(inode, fieinfo, get_block);
inode_unlock(inode);
return ret;
}
diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
index 671085512e0f..1f37d086371c 100644
--- a/fs/nilfs2/inode.c
+++ b/fs/nilfs2/inode.c
@@ -992,9 +992,10 @@ void nilfs_dirty_inode(struct inode *inode, int flags)
nilfs_transaction_commit(inode->i_sb); /* never fails */
}
-int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- __u64 start, __u64 len)
+int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
+ u64 start = fieinfo->fi_start;
+ u64 len = fieinfo->fi_len;
struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
__u64 logical = 0, phys = 0, size = 0;
__u32 flags = 0;
diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h
index a2f247b6a209..55d1307ed710 100644
--- a/fs/nilfs2/nilfs.h
+++ b/fs/nilfs2/nilfs.h
@@ -276,8 +276,7 @@ extern int nilfs_inode_dirty(struct inode *);
int nilfs_set_file_dirty(struct inode *inode, unsigned int nr_dirty);
extern int __nilfs_mark_inode_dirty(struct inode *, int);
extern void nilfs_dirty_inode(struct inode *, int flags);
-int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- __u64 start, __u64 len);
+int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo);
static inline int nilfs_mark_inode_dirty(struct inode *inode)
{
return __nilfs_mark_inode_dirty(inode, I_DIRTY);
diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
index 06cb96462bf9..e01fd38ea935 100644
--- a/fs/ocfs2/extent_map.c
+++ b/fs/ocfs2/extent_map.c
@@ -749,8 +749,7 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh,
#define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
-int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- u64 map_start, u64 map_len)
+int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
int ret, is_last;
u32 mapping_end, cpos;
@@ -759,6 +758,8 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
u64 len_bytes, phys_bytes, virt_bytes;
struct buffer_head *di_bh = NULL;
struct ocfs2_extent_rec rec;
+ u64 map_start = fieinfo->fi_start;
+ u64 map_len = fieinfo->fi_len;
ret = fiemap_check_flags(fieinfo, OCFS2_FIEMAP_FLAGS);
if (ret)
diff --git a/fs/ocfs2/extent_map.h b/fs/ocfs2/extent_map.h
index 1057586ec19f..793be96099c0 100644
--- a/fs/ocfs2/extent_map.h
+++ b/fs/ocfs2/extent_map.h
@@ -50,8 +50,7 @@ int ocfs2_get_clusters(struct inode *inode, u32 v_cluster, u32 *p_cluster,
int ocfs2_extent_map_get_blocks(struct inode *inode, u64 v_blkno, u64 *p_blkno,
u64 *ret_count, unsigned int *extent_flags);
-int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- u64 map_start, u64 map_len);
+int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo);
int ocfs2_overwrite_io(struct inode *inode, struct buffer_head *di_bh,
u64 map_start, u64 map_len);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 3b7ed5d2279c..6f00b0ef6b43 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -456,8 +456,7 @@ int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags)
return 0;
}
-static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- u64 start, u64 len)
+static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
int err;
struct inode *realinode = ovl_inode_real(inode);
@@ -471,7 +470,7 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
filemap_write_and_wait(realinode->i_mapping);
- err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
+ err = realinode->i_op->fiemap(realinode, fieinfo);
revert_creds(old_cred);
return err;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 74047bd0c1ae..1f4354fa989b 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1109,12 +1109,12 @@ xfs_vn_update_time(
STATIC int
xfs_vn_fiemap(
- struct inode *inode,
- struct fiemap_extent_info *fieinfo,
- u64 start,
- u64 length)
+ struct inode *inode,
+ struct fiemap_extent_info *fieinfo)
{
- int error;
+ u64 start = fieinfo->fi_start;
+ u64 length = fieinfo->fi_len;
+ int error;
xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d5e7c744aea6..7b744b7de24e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1705,11 +1705,14 @@ extern bool may_open_dev(const struct path *path);
* VFS FS_IOC_FIEMAP helper definitions.
*/
struct fiemap_extent_info {
- unsigned int fi_flags; /* Flags as passed from user */
- unsigned int fi_extents_mapped; /* Number of mapped extents */
- unsigned int fi_extents_max; /* Size of fiemap_extent array */
- struct fiemap_extent __user *fi_extents_start; /* Start of
- fiemap_extent array */
+ unsigned int fi_flags; /* Flags as passed from user */
+ u64 fi_start;
+ u64 fi_len;
+ unsigned int fi_extents_mapped; /* Number of mapped extents */
+ unsigned int fi_extents_max; /* Size of fiemap_extent array */
+ struct fiemap_extent __user *fi_extents_start; /* Start of
+ fiemap_extent
+ array */
};
int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
u64 phys, u64 len, u32 flags);
@@ -1841,8 +1844,7 @@ struct inode_operations {
int (*setattr) (struct dentry *, struct iattr *);
int (*getattr) (const struct path *, struct kstat *, u32, unsigned int);
ssize_t (*listxattr) (struct dentry *, char *, size_t);
- int (*fiemap)(struct inode *, struct fiemap_extent_info *, u64 start,
- u64 len);
+ int (*fiemap)(struct inode *, struct fiemap_extent_info *);
int (*update_time)(struct inode *, struct timespec64 *, int);
int (*atomic_open)(struct inode *, struct dentry *,
struct file *, unsigned open_flag,
@@ -3199,11 +3201,10 @@ extern int vfs_readlink(struct dentry *, char __user *, int);
extern int __generic_block_fiemap(struct inode *inode,
struct fiemap_extent_info *fieinfo,
- loff_t start, loff_t len,
get_block_t *get_block);
extern int generic_block_fiemap(struct inode *inode,
- struct fiemap_extent_info *fieinfo, u64 start,
- u64 len, get_block_t *get_block);
+ struct fiemap_extent_info *fieinfo,
+ get_block_t *get_block);
extern struct file_system_type *get_filesystem(struct file_system_type *fs);
extern void put_filesystem(struct file_system_type *fs);
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 6/9] iomap: Remove length and start fields from iomap_fiemap
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
` (4 preceding siblings ...)
2019-07-31 14:12 ` [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info Carlos Maiolino
@ 2019-07-31 14:12 ` Carlos Maiolino
2019-07-31 23:24 ` Darrick J. Wong
2019-07-31 14:12 ` [PATCH 7/9] fiemap: Use a callback to fill fiemap extents Carlos Maiolino
` (3 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-07-31 14:12 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
fiemap_extent_info now embeds start and length parameters, users of
iomap_fiemap() doesn't need to pass it individually anymore.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
fs/gfs2/inode.c | 4 +---
fs/iomap.c | 4 +++-
fs/xfs/xfs_iops.c | 8 ++------
include/linux/iomap.h | 2 +-
4 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 5e84d5963506..df31bd8ecf6f 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -2006,8 +2006,6 @@ static int gfs2_getattr(const struct path *path, struct kstat *stat,
static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
- u64 start = fieinfo->fi_start;
- u64 len = fieinfo->fi_len;
struct gfs2_inode *ip = GFS2_I(inode);
struct gfs2_holder gh;
int ret;
@@ -2018,7 +2016,7 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
if (ret)
goto out;
- ret = iomap_fiemap(inode, fieinfo, start, len, &gfs2_iomap_ops);
+ ret = iomap_fiemap(inode, fieinfo, &gfs2_iomap_ops);
gfs2_glock_dq_uninit(&gh);
diff --git a/fs/iomap.c b/fs/iomap.c
index 97cb9d486a7d..b1e88722e10b 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -1184,9 +1184,11 @@ iomap_fiemap_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
}
int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
- loff_t start, loff_t len, const struct iomap_ops *ops)
+ const struct iomap_ops *ops)
{
struct fiemap_ctx ctx;
+ loff_t start = fi->fi_start;
+ loff_t len = fi->fi_len;
loff_t ret;
memset(&ctx, 0, sizeof(ctx));
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 1f4354fa989b..b485190b7ecd 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1112,18 +1112,14 @@ xfs_vn_fiemap(
struct inode *inode,
struct fiemap_extent_info *fieinfo)
{
- u64 start = fieinfo->fi_start;
- u64 length = fieinfo->fi_len;
int error;
xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;
- error = iomap_fiemap(inode, fieinfo, start, length,
- &xfs_xattr_iomap_ops);
+ error = iomap_fiemap(inode, fieinfo, &xfs_xattr_iomap_ops);
} else {
- error = iomap_fiemap(inode, fieinfo, start, length,
- &xfs_iomap_ops);
+ error = iomap_fiemap(inode, fieinfo, &xfs_iomap_ops);
}
xfs_iunlock(XFS_I(inode), XFS_IOLOCK_SHARED);
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 0fefb5455bda..bc4c421ee822 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -145,7 +145,7 @@ int iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf,
const struct iomap_ops *ops);
int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- loff_t start, loff_t len, const struct iomap_ops *ops);
+ const struct iomap_ops *ops);
loff_t iomap_seek_hole(struct inode *inode, loff_t offset,
const struct iomap_ops *ops);
loff_t iomap_seek_data(struct inode *inode, loff_t offset,
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 7/9] fiemap: Use a callback to fill fiemap extents
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
` (5 preceding siblings ...)
2019-07-31 14:12 ` [PATCH 6/9] iomap: Remove length and start fields from iomap_fiemap Carlos Maiolino
@ 2019-07-31 14:12 ` Carlos Maiolino
2019-07-31 23:26 ` Darrick J. Wong
2019-07-31 14:12 ` [PATCH 8/9] Use FIEMAP for FIBMAP calls Carlos Maiolino
` (2 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-07-31 14:12 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
As a goal to enable fiemap infrastructure to be used by fibmap too, we need a
way to use different helpers to fill extent data, depending on its usage. One
helper to fill extent data stored in user address space (used in fiemap), and
another fo fill extent data stored in kernel address space (will be used in
fibmap).
This patch sets up the usage of a callback to be used to fill in the extents.
It transforms the current fiemap_fill_next_extent, into a simple helper to call
the callback, avoiding unneeded changes on any filesystem, and reutilizes the
original function as the callback used by FIEMAP.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
Changelog:
V3:
- Rebase to current linux-next
- Fix conflict on rebase
- Merge this patch into patch 07 from V2
- Rename fi_extents_start to fi_cb_data
V2:
- Now based on the rework on fiemap_extent_info (previous was
based on fiemap_ctx)
fs/ioctl.c | 45 ++++++++++++++++++++++++++-------------------
include/linux/fs.h | 12 +++++++++---
2 files changed, 35 insertions(+), 22 deletions(-)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index ad8edcb10dc9..d72696c222de 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -77,29 +77,14 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
return error;
}
-/**
- * fiemap_fill_next_extent - Fiemap helper function
- * @fieinfo: Fiemap context passed into ->fiemap
- * @logical: Extent logical start offset, in bytes
- * @phys: Extent physical start offset, in bytes
- * @len: Extent length, in bytes
- * @flags: FIEMAP_EXTENT flags that describe this extent
- *
- * Called from file system ->fiemap callback. Will populate extent
- * info as passed in via arguments and copy to user memory. On
- * success, extent count on fieinfo is incremented.
- *
- * Returns 0 on success, -errno on error, 1 if this was the last
- * extent that will fit in user array.
- */
#define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC)
#define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED)
#define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE)
-int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
+int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
u64 phys, u64 len, u32 flags)
{
struct fiemap_extent extent;
- struct fiemap_extent __user *dest = fieinfo->fi_extents_start;
+ struct fiemap_extent __user *dest = fieinfo->fi_cb_data;
/* only count the extents */
if (fieinfo->fi_extents_max == 0) {
@@ -132,6 +117,27 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
return 1;
return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
}
+
+/**
+ * fiemap_fill_next_extent - Fiemap helper function
+ * @fieinfo: Fiemap context passed into ->fiemap
+ * @logical: Extent logical start offset, in bytes
+ * @phys: Extent physical start offset, in bytes
+ * @len: Extent length, in bytes
+ * @flags: FIEMAP_EXTENT flags that describe this extent
+ *
+ * Called from file system ->fiemap callback. Will populate extent
+ * info as passed in via arguments and copy to user memory. On
+ * success, extent count on fieinfo is incremented.
+ *
+ * Returns 0 on success, -errno on error, 1 if this was the last
+ * extent that will fit in user array.
+ */
+int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
+ u64 phys, u64 len, u32 flags)
+{
+ return fieinfo->fi_cb(fieinfo, logical, phys, len, flags);
+}
EXPORT_SYMBOL(fiemap_fill_next_extent);
/**
@@ -209,12 +215,13 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
fieinfo.fi_flags = fiemap.fm_flags;
fieinfo.fi_extents_max = fiemap.fm_extent_count;
- fieinfo.fi_extents_start = ufiemap->fm_extents;
+ fieinfo.fi_cb_data = ufiemap->fm_extents;
fieinfo.fi_start = fiemap.fm_start;
fieinfo.fi_len = len;
+ fieinfo.fi_cb = fiemap_fill_user_extent;
if (fiemap.fm_extent_count != 0 &&
- !access_ok(fieinfo.fi_extents_start,
+ !access_ok(fieinfo.fi_cb_data,
fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
return -EFAULT;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7b744b7de24e..a8bd3c4f6d86 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -66,6 +66,7 @@ struct fscrypt_info;
struct fscrypt_operations;
struct fs_context;
struct fs_parameter_description;
+struct fiemap_extent_info;
extern void __init inode_init(void);
extern void __init inode_init_early(void);
@@ -1704,16 +1705,21 @@ extern bool may_open_dev(const struct path *path);
/*
* VFS FS_IOC_FIEMAP helper definitions.
*/
+
+typedef int (*fiemap_fill_cb)(struct fiemap_extent_info *fieinfo, u64 logical,
+ u64 phys, u64 len, u32 flags);
+
struct fiemap_extent_info {
unsigned int fi_flags; /* Flags as passed from user */
u64 fi_start;
u64 fi_len;
unsigned int fi_extents_mapped; /* Number of mapped extents */
unsigned int fi_extents_max; /* Size of fiemap_extent array */
- struct fiemap_extent __user *fi_extents_start; /* Start of
- fiemap_extent
- array */
+ void *fi_cb_data; /* Start of fiemap_extent
+ array */
+ fiemap_fill_cb fi_cb;
};
+
int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
u64 phys, u64 len, u32 flags);
int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 8/9] Use FIEMAP for FIBMAP calls
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
` (6 preceding siblings ...)
2019-07-31 14:12 ` [PATCH 7/9] fiemap: Use a callback to fill fiemap extents Carlos Maiolino
@ 2019-07-31 14:12 ` Carlos Maiolino
2019-07-31 23:22 ` Darrick J. Wong
2019-07-31 14:12 ` [PATCH 9/9] xfs: Get rid of ->bmap Carlos Maiolino
2019-08-02 10:20 ` [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
9 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-07-31 14:12 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
Enables the usage of FIEMAP ioctl infrastructure to handle FIBMAP calls.
From now on, ->bmap() methods can start to be removed from filesystems
which already provides ->fiemap().
This adds a new helper - bmap_fiemap() - which is used to fill in the
fiemap request, call into the underlying filesystem and check the flags
set in the extent requested.
Add a new fiemap fill extent callback to handle the in-kernel only
fiemap_extent structure used for FIBMAP.
The new FIEMAP_KERNEL_FIBMAP flag, is used to tell the filesystem
->fiemap interface, that the call is coming from ioctl_fibmap. The
addition of this new flag, requires an update to fiemap_check_flags(),
so it doesn't treat FIBMAP requests as invalid.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
Changelog:
V4:
- Fix if conditional in bmap()
- Add filesystem-specific modifications
V3:
- Add FIEMAP_EXTENT_SHARED to the list of invalid extents in
bmap_fiemap()
- Rename fi_extents_start to fi_cb_data
- Use if conditional instead of ternary operator
- Make fiemap_fill_* callbacks static (which required the
removal of some macros
- Set FIEMAP_FLAG_SYNC when calling in ->fiemap method from
fibmap
- Add FIEMAP_KERNEL_FIBMAP flag, to identify the usage of fiemap
infrastructure for fibmap calls, defined in fs.h so it's not
exported to userspace.
- Update fiemap_check_flags() to understand FIEMAP_KERNEL_FIBMAP
- Update filesystems supporting both FIBMAP and FIEMAP, which
need extra checks on FIBMAP calls
V2:
- Now based on the updated fiemap_extent_info,
- move the fiemap call itself to a new helper
fs/ext4/extents.c | 7 +++-
fs/f2fs/data.c | 10 +++++-
fs/gfs2/inode.c | 6 +++-
fs/inode.c | 81 +++++++++++++++++++++++++++++++++++++++++--
fs/ioctl.c | 40 ++++++++++++++-------
fs/iomap.c | 2 +-
fs/ocfs2/extent_map.c | 8 ++++-
fs/xfs/xfs_iops.c | 5 +++
include/linux/fs.h | 4 +++
9 files changed, 144 insertions(+), 19 deletions(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 436e564ebdd6..093b6a07067f 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5001,7 +5001,9 @@ static int ext4_find_delayed_extent(struct inode *inode,
return next_del;
}
/* fiemap flags we can handle specified here */
-#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC|FIEMAP_FLAG_XATTR)
+#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC | \
+ FIEMAP_FLAG_XATTR| \
+ FIEMAP_KERNEL_FIBMAP)
static int ext4_xattr_fiemap(struct inode *inode,
struct fiemap_extent_info *fieinfo)
@@ -5048,6 +5050,9 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
if (ext4_has_inline_data(inode)) {
int has_inline = 1;
+ if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
+ return -EINVAL;
+
error = ext4_inline_data_fiemap(inode, fieinfo, &has_inline,
start, len);
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 2979ca40d192..29b6c48fb6cc 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1409,6 +1409,9 @@ static int f2fs_xattr_fiemap(struct inode *inode,
return (err < 0 ? err : 0);
}
+#define F2FS_FIEMAP_COMPAT (FIEMAP_FLAG_SYNC | \
+ FIEMAP_FLAG_XATTR| \
+ FIEMAP_KERNEL_FIBMAP)
int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
u64 start = fieinfo->fi_start;
@@ -1426,7 +1429,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
return ret;
}
- ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
+ ret = fiemap_check_flags(fieinfo, F2FS_FIEMAP_COMPAT);
if (ret)
return ret;
@@ -1438,6 +1441,11 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
}
if (f2fs_has_inline_data(inode)) {
+
+ ret = -EINVAL;
+ if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
+ goto out;
+
ret = f2fs_inline_data_fiemap(inode, fieinfo, start, len);
if (ret != -EAGAIN)
goto out;
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index df31bd8ecf6f..30554b4f49c3 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -2016,7 +2016,11 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
if (ret)
goto out;
- ret = iomap_fiemap(inode, fieinfo, &gfs2_iomap_ops);
+ if (gfs2_is_stuffed(ip) &&
+ (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP))
+ ret = -EINVAL;
+ else
+ ret = iomap_fiemap(inode, fieinfo, &gfs2_iomap_ops);
gfs2_glock_dq_uninit(&gh);
diff --git a/fs/inode.c b/fs/inode.c
index 824fa54d393d..02552b09e77f 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1575,6 +1575,78 @@ void iput(struct inode *inode)
}
EXPORT_SYMBOL(iput);
+static int fiemap_fill_kernel_extent(struct fiemap_extent_info *fieinfo,
+ u64 logical, u64 phys, u64 len, u32 flags)
+{
+ struct fiemap_extent *extent = fieinfo->fi_cb_data;
+
+ /* only count the extents */
+ if (fieinfo->fi_cb_data == 0) {
+ fieinfo->fi_extents_mapped++;
+ goto out;
+ }
+
+ if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
+ return 1;
+
+ if (flags & FIEMAP_EXTENT_DELALLOC)
+ flags |= FIEMAP_EXTENT_UNKNOWN;
+ if (flags & FIEMAP_EXTENT_DATA_ENCRYPTED)
+ flags |= FIEMAP_EXTENT_ENCODED;
+ if (flags & (FIEMAP_EXTENT_DATA_TAIL | FIEMAP_EXTENT_DATA_INLINE))
+ flags |= FIEMAP_EXTENT_NOT_ALIGNED;
+
+ extent->fe_logical = logical;
+ extent->fe_physical = phys;
+ extent->fe_length = len;
+ extent->fe_flags = flags;
+
+ fieinfo->fi_extents_mapped++;
+
+ if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max)
+ return 1;
+
+out:
+ if (flags & FIEMAP_EXTENT_LAST)
+ return 1;
+ return 0;
+}
+
+static int bmap_fiemap(struct inode *inode, sector_t *block)
+{
+ struct fiemap_extent_info fieinfo = { 0, };
+ struct fiemap_extent fextent;
+ u64 start = *block << inode->i_blkbits;
+ int error = -EINVAL;
+
+ fextent.fe_logical = 0;
+ fextent.fe_physical = 0;
+ fieinfo.fi_extents_max = 1;
+ fieinfo.fi_extents_mapped = 0;
+ fieinfo.fi_cb_data = &fextent;
+ fieinfo.fi_start = start;
+ fieinfo.fi_len = 1 << inode->i_blkbits;
+ fieinfo.fi_cb = fiemap_fill_kernel_extent;
+ fieinfo.fi_flags = (FIEMAP_KERNEL_FIBMAP | FIEMAP_FLAG_SYNC);
+
+ error = inode->i_op->fiemap(inode, &fieinfo);
+
+ if (error)
+ return error;
+
+ if (fieinfo.fi_flags & (FIEMAP_EXTENT_UNKNOWN |
+ FIEMAP_EXTENT_ENCODED |
+ FIEMAP_EXTENT_DATA_INLINE |
+ FIEMAP_EXTENT_UNWRITTEN |
+ FIEMAP_EXTENT_SHARED))
+ return -EINVAL;
+
+ *block = (fextent.fe_physical +
+ (start - fextent.fe_logical)) >> inode->i_blkbits;
+
+ return error;
+}
+
/**
* bmap - find a block number in a file
* @inode: inode owning the block number being requested
@@ -1591,10 +1663,15 @@ EXPORT_SYMBOL(iput);
*/
int bmap(struct inode *inode, sector_t *block)
{
- if (!inode->i_mapping->a_ops->bmap)
+ if (inode->i_op->fiemap)
+ return bmap_fiemap(inode, block);
+
+ if (inode->i_mapping->a_ops->bmap)
+ *block = inode->i_mapping->a_ops->bmap(inode->i_mapping,
+ *block);
+ else
return -EINVAL;
- *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
return 0;
}
EXPORT_SYMBOL(bmap);
diff --git a/fs/ioctl.c b/fs/ioctl.c
index d72696c222de..0759ac6e4c7e 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -77,11 +77,8 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
return error;
}
-#define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC)
-#define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED)
-#define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE)
-int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
- u64 phys, u64 len, u32 flags)
+static int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo,
+ u64 logical, u64 phys, u64 len, u32 flags)
{
struct fiemap_extent extent;
struct fiemap_extent __user *dest = fieinfo->fi_cb_data;
@@ -89,17 +86,17 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
/* only count the extents */
if (fieinfo->fi_extents_max == 0) {
fieinfo->fi_extents_mapped++;
- return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
+ goto out;
}
if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
return 1;
- if (flags & SET_UNKNOWN_FLAGS)
+ if (flags & FIEMAP_EXTENT_DELALLOC)
flags |= FIEMAP_EXTENT_UNKNOWN;
- if (flags & SET_NO_UNMOUNTED_IO_FLAGS)
+ if (flags & FIEMAP_EXTENT_DATA_ENCRYPTED)
flags |= FIEMAP_EXTENT_ENCODED;
- if (flags & SET_NOT_ALIGNED_FLAGS)
+ if (flags & (FIEMAP_EXTENT_DATA_TAIL | FIEMAP_EXTENT_DATA_INLINE))
flags |= FIEMAP_EXTENT_NOT_ALIGNED;
memset(&extent, 0, sizeof(extent));
@@ -115,7 +112,11 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
fieinfo->fi_extents_mapped++;
if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max)
return 1;
- return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
+
+out:
+ if (flags & FIEMAP_EXTENT_LAST)
+ return 1;
+ return 0;
}
/**
@@ -151,13 +152,23 @@ EXPORT_SYMBOL(fiemap_fill_next_extent);
* flags, the invalid values will be written into the fieinfo structure, and
* -EBADR is returned, which tells ioctl_fiemap() to return those values to
* userspace. For this reason, a return code of -EBADR should be preserved.
+ * In case ->fiemap is being used for FIBMAP calls, and the filesystem does not
+ * support it, return -EINVAL.
*
- * Returns 0 on success, -EBADR on bad flags.
+ * Returns 0 on success, -EBADR on bad flags, -EINVAL for an unsupported FIBMAP
+ * request.
*/
int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags)
{
u32 incompat_flags;
+ if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) {
+ if (fs_flags & FIEMAP_KERNEL_FIBMAP)
+ return 0;
+
+ return -EINVAL;
+ }
+
incompat_flags = fieinfo->fi_flags & ~(FIEMAP_FLAGS_COMPAT & fs_flags);
if (incompat_flags) {
fieinfo->fi_flags = incompat_flags;
@@ -208,6 +219,10 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
return -EINVAL;
+ /* Userspace has no access to this flag */
+ if (fiemap.fm_flags & FIEMAP_KERNEL_FIBMAP)
+ return -EINVAL;
+
error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
&len);
if (error)
@@ -318,7 +333,8 @@ int __generic_block_fiemap(struct inode *inode,
bool past_eof = false, whole_file = false;
int ret = 0;
- ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
+ ret = fiemap_check_flags(fieinfo,
+ FIEMAP_FLAG_SYNC | FIEMAP_KERNEL_FIBMAP);
if (ret)
return ret;
diff --git a/fs/iomap.c b/fs/iomap.c
index b1e88722e10b..2b182abd18e8 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -1195,7 +1195,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
ctx.fi = fi;
ctx.prev.type = IOMAP_HOLE;
- ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC);
+ ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC | FIEMAP_KERNEL_FIBMAP);
if (ret)
return ret;
diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
index e01fd38ea935..2884395f3972 100644
--- a/fs/ocfs2/extent_map.c
+++ b/fs/ocfs2/extent_map.c
@@ -747,7 +747,7 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh,
return 0;
}
-#define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
+#define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC | FIEMAP_KERNEL_FIBMAP)
int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
{
@@ -756,6 +756,7 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
unsigned int hole_size;
struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
u64 len_bytes, phys_bytes, virt_bytes;
+
struct buffer_head *di_bh = NULL;
struct ocfs2_extent_rec rec;
u64 map_start = fieinfo->fi_start;
@@ -765,6 +766,11 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
if (ret)
return ret;
+ if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) {
+ if (ocfs2_is_refcount_inode(inode))
+ return -EINVAL;
+ }
+
ret = ocfs2_inode_lock(inode, &di_bh, 0);
if (ret) {
mlog_errno(ret);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b485190b7ecd..18a798e9076b 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1113,6 +1113,11 @@ xfs_vn_fiemap(
struct fiemap_extent_info *fieinfo)
{
int error;
+ struct xfs_inode *ip = XFS_I(inode);
+
+ if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
+ if (xfs_is_reflink_inode(ip) || XFS_IS_REALTIME_INODE(ip))
+ return -EINVAL;
xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a8bd3c4f6d86..233e12ccb6d3 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1709,6 +1709,10 @@ extern bool may_open_dev(const struct path *path);
typedef int (*fiemap_fill_cb)(struct fiemap_extent_info *fieinfo, u64 logical,
u64 phys, u64 len, u32 flags);
+#define FIEMAP_KERNEL_FIBMAP 0x10000000 /* FIBMAP call through FIEMAP
+ interface. This is a kernel
+ only flag */
+
struct fiemap_extent_info {
unsigned int fi_flags; /* Flags as passed from user */
u64 fi_start;
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 9/9] xfs: Get rid of ->bmap
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
` (7 preceding siblings ...)
2019-07-31 14:12 ` [PATCH 8/9] Use FIEMAP for FIBMAP calls Carlos Maiolino
@ 2019-07-31 14:12 ` Carlos Maiolino
2019-07-31 23:30 ` Darrick J. Wong
2019-08-02 10:20 ` [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
9 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-07-31 14:12 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
We don't need ->bmap anymore, only usage for it was FIBMAP, which is now
gone.
Also kill iomap_bmap() and iomap_bmap_actor once it has no users anymore.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
Changelog:
V2:
- Kill iomap_bmap() and iomap_bmap_actor()
fs/iomap.c | 34 ----------------------------------
fs/xfs/xfs_aops.c | 24 ------------------------
fs/xfs/xfs_trace.h | 1 -
3 files changed, 59 deletions(-)
diff --git a/fs/iomap.c b/fs/iomap.c
index 2b182abd18e8..12e6a575feb4 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -2153,37 +2153,3 @@ int iomap_swapfile_activate(struct swap_info_struct *sis,
}
EXPORT_SYMBOL_GPL(iomap_swapfile_activate);
#endif /* CONFIG_SWAP */
-
-static loff_t
-iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,
- void *data, struct iomap *iomap)
-{
- sector_t *bno = data, addr;
-
- if (iomap->type == IOMAP_MAPPED) {
- addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits;
- if (addr > INT_MAX)
- WARN(1, "would truncate bmap result\n");
- else
- *bno = addr;
- }
- return 0;
-}
-
-/* legacy ->bmap interface. 0 is the error return (!) */
-sector_t
-iomap_bmap(struct address_space *mapping, sector_t bno,
- const struct iomap_ops *ops)
-{
- struct inode *inode = mapping->host;
- loff_t pos = bno << inode->i_blkbits;
- unsigned blocksize = i_blocksize(inode);
-
- if (filemap_write_and_wait(mapping))
- return 0;
-
- bno = 0;
- iomap_apply(inode, pos, blocksize, 0, ops, &bno, iomap_bmap_actor);
- return bno;
-}
-EXPORT_SYMBOL_GPL(iomap_bmap);
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 3619e9e8d359..76ee495eba1a 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -1006,29 +1006,6 @@ xfs_vm_releasepage(
return iomap_releasepage(page, gfp_mask);
}
-STATIC sector_t
-xfs_vm_bmap(
- struct address_space *mapping,
- sector_t block)
-{
- struct xfs_inode *ip = XFS_I(mapping->host);
-
- trace_xfs_vm_bmap(ip);
-
- /*
- * The swap code (ab-)uses ->bmap to get a block mapping and then
- * bypasses the file system for actual I/O. We really can't allow
- * that on reflinks inodes, so we have to skip out here. And yes,
- * 0 is the magic code for a bmap error.
- *
- * Since we don't pass back blockdev info, we can't return bmap
- * information for rt files either.
- */
- if (xfs_is_cow_inode(ip) || XFS_IS_REALTIME_INODE(ip))
- return 0;
- return iomap_bmap(mapping, block, &xfs_iomap_ops);
-}
-
STATIC int
xfs_vm_readpage(
struct file *unused,
@@ -1067,7 +1044,6 @@ const struct address_space_operations xfs_address_space_operations = {
.set_page_dirty = iomap_set_page_dirty,
.releasepage = xfs_vm_releasepage,
.invalidatepage = xfs_vm_invalidatepage,
- .bmap = xfs_vm_bmap,
.direct_IO = noop_direct_IO,
.migratepage = iomap_migrate_page,
.is_partially_uptodate = iomap_is_partially_uptodate,
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 47fb07d86efd..3a45a3971dce 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -621,7 +621,6 @@ DEFINE_INODE_EVENT(xfs_readdir);
#ifdef CONFIG_XFS_POSIX_ACL
DEFINE_INODE_EVENT(xfs_get_acl);
#endif
-DEFINE_INODE_EVENT(xfs_vm_bmap);
DEFINE_INODE_EVENT(xfs_file_ioctl);
DEFINE_INODE_EVENT(xfs_file_compat_ioctl);
DEFINE_INODE_EVENT(xfs_ioctl_setattr);
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-07-31 14:12 ` [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap Carlos Maiolino
@ 2019-07-31 23:12 ` Darrick J. Wong
2019-08-02 9:19 ` Carlos Maiolino
0 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2019-07-31 23:12 UTC (permalink / raw)
To: Carlos Maiolino
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Wed, Jul 31, 2019 at 04:12:40PM +0200, Carlos Maiolino wrote:
> Now we have the possibility of proper error return in bmap, use bmap()
> function in ioctl_fibmap() instead of calling ->bmap method directly.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>
> Changelog:
>
> V4:
> - Ensure ioctl_fibmap() returns 0 in case of error returned from
> bmap(). Otherwise we'll be changing the user interface (which
> returns 0 in case of error)
> V3:
> - Rename usr_blk to ur_block
>
> V2:
> - Use a local sector_t variable to asign the block number
> instead of using direct casting.
>
> fs/ioctl.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index fef3a6bf7c78..6b589c873bc2 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -53,19 +53,28 @@ EXPORT_SYMBOL(vfs_ioctl);
>
> static int ioctl_fibmap(struct file *filp, int __user *p)
> {
> - struct address_space *mapping = filp->f_mapping;
> - int res, block;
> + struct inode *inode = file_inode(filp);
> + int error, ur_block;
> + sector_t block;
>
> - /* do we support this mess? */
> - if (!mapping->a_ops->bmap)
> - return -EINVAL;
> if (!capable(CAP_SYS_RAWIO))
> return -EPERM;
> - res = get_user(block, p);
> - if (res)
> - return res;
> - res = mapping->a_ops->bmap(mapping, block);
> - return put_user(res, p);
> +
> + error = get_user(ur_block, p);
> + if (error)
> + return error;
> +
> + block = ur_block;
> + error = bmap(inode, &block);
> +
> + if (error)
> + ur_block = 0;
> + else
> + ur_block = block;
What happens if ur_block > INT_MAX? Shouldn't we return zero (i.e.
error) instead of truncating the value? Maybe the code does this
somewhere else? Here seemed like the obvious place for an overflow
check as we go from sector_t to int.
--D
> +
> + error = put_user(ur_block, p);
> +
> + return error;
> }
>
> /**
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] Use FIEMAP for FIBMAP calls
2019-07-31 14:12 ` [PATCH 8/9] Use FIEMAP for FIBMAP calls Carlos Maiolino
@ 2019-07-31 23:22 ` Darrick J. Wong
2019-07-31 23:31 ` Darrick J. Wong
2019-08-02 13:48 ` Carlos Maiolino
0 siblings, 2 replies; 47+ messages in thread
From: Darrick J. Wong @ 2019-07-31 23:22 UTC (permalink / raw)
To: Carlos Maiolino
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Wed, Jul 31, 2019 at 04:12:44PM +0200, Carlos Maiolino wrote:
> Enables the usage of FIEMAP ioctl infrastructure to handle FIBMAP calls.
> From now on, ->bmap() methods can start to be removed from filesystems
> which already provides ->fiemap().
>
> This adds a new helper - bmap_fiemap() - which is used to fill in the
> fiemap request, call into the underlying filesystem and check the flags
> set in the extent requested.
>
> Add a new fiemap fill extent callback to handle the in-kernel only
> fiemap_extent structure used for FIBMAP.
>
> The new FIEMAP_KERNEL_FIBMAP flag, is used to tell the filesystem
> ->fiemap interface, that the call is coming from ioctl_fibmap. The
> addition of this new flag, requires an update to fiemap_check_flags(),
> so it doesn't treat FIBMAP requests as invalid.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>
> Changelog:
>
> V4:
> - Fix if conditional in bmap()
> - Add filesystem-specific modifications
> V3:
> - Add FIEMAP_EXTENT_SHARED to the list of invalid extents in
> bmap_fiemap()
> - Rename fi_extents_start to fi_cb_data
> - Use if conditional instead of ternary operator
> - Make fiemap_fill_* callbacks static (which required the
> removal of some macros
> - Set FIEMAP_FLAG_SYNC when calling in ->fiemap method from
> fibmap
> - Add FIEMAP_KERNEL_FIBMAP flag, to identify the usage of fiemap
> infrastructure for fibmap calls, defined in fs.h so it's not
> exported to userspace.
> - Update fiemap_check_flags() to understand FIEMAP_KERNEL_FIBMAP
> - Update filesystems supporting both FIBMAP and FIEMAP, which
> need extra checks on FIBMAP calls
>
> V2:
> - Now based on the updated fiemap_extent_info,
> - move the fiemap call itself to a new helper
>
> fs/ext4/extents.c | 7 +++-
> fs/f2fs/data.c | 10 +++++-
> fs/gfs2/inode.c | 6 +++-
> fs/inode.c | 81 +++++++++++++++++++++++++++++++++++++++++--
> fs/ioctl.c | 40 ++++++++++++++-------
> fs/iomap.c | 2 +-
> fs/ocfs2/extent_map.c | 8 ++++-
> fs/xfs/xfs_iops.c | 5 +++
> include/linux/fs.h | 4 +++
> 9 files changed, 144 insertions(+), 19 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 436e564ebdd6..093b6a07067f 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -5001,7 +5001,9 @@ static int ext4_find_delayed_extent(struct inode *inode,
> return next_del;
> }
> /* fiemap flags we can handle specified here */
> -#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC|FIEMAP_FLAG_XATTR)
> +#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC | \
> + FIEMAP_FLAG_XATTR| \
> + FIEMAP_KERNEL_FIBMAP)
>
> static int ext4_xattr_fiemap(struct inode *inode,
> struct fiemap_extent_info *fieinfo)
> @@ -5048,6 +5050,9 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> if (ext4_has_inline_data(inode)) {
> int has_inline = 1;
>
> + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> + return -EINVAL;
Wouldn't the inline data case be caught by fiemap_bmap and turned into
-EINVAL?
> +
> error = ext4_inline_data_fiemap(inode, fieinfo, &has_inline,
> start, len);
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 2979ca40d192..29b6c48fb6cc 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1409,6 +1409,9 @@ static int f2fs_xattr_fiemap(struct inode *inode,
> return (err < 0 ? err : 0);
> }
>
> +#define F2FS_FIEMAP_COMPAT (FIEMAP_FLAG_SYNC | \
> + FIEMAP_FLAG_XATTR| \
> + FIEMAP_KERNEL_FIBMAP)
> int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> u64 start = fieinfo->fi_start;
> @@ -1426,7 +1429,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> return ret;
> }
>
> - ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
> + ret = fiemap_check_flags(fieinfo, F2FS_FIEMAP_COMPAT);
> if (ret)
> return ret;
>
> @@ -1438,6 +1441,11 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> }
>
> if (f2fs_has_inline_data(inode)) {
> +
> + ret = -EINVAL;
> + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> + goto out;
> +
> ret = f2fs_inline_data_fiemap(inode, fieinfo, start, len);
> if (ret != -EAGAIN)
> goto out;
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index df31bd8ecf6f..30554b4f49c3 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -2016,7 +2016,11 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> if (ret)
> goto out;
>
> - ret = iomap_fiemap(inode, fieinfo, &gfs2_iomap_ops);
> + if (gfs2_is_stuffed(ip) &&
> + (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP))
> + ret = -EINVAL;
> + else
> + ret = iomap_fiemap(inode, fieinfo, &gfs2_iomap_ops);
>
> gfs2_glock_dq_uninit(&gh);
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 824fa54d393d..02552b09e77f 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1575,6 +1575,78 @@ void iput(struct inode *inode)
> }
> EXPORT_SYMBOL(iput);
>
> +static int fiemap_fill_kernel_extent(struct fiemap_extent_info *fieinfo,
> + u64 logical, u64 phys, u64 len, u32 flags)
> +{
> + struct fiemap_extent *extent = fieinfo->fi_cb_data;
> +
> + /* only count the extents */
> + if (fieinfo->fi_cb_data == 0) {
> + fieinfo->fi_extents_mapped++;
> + goto out;
> + }
> +
> + if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
> + return 1;
> +
> + if (flags & FIEMAP_EXTENT_DELALLOC)
> + flags |= FIEMAP_EXTENT_UNKNOWN;
> + if (flags & FIEMAP_EXTENT_DATA_ENCRYPTED)
> + flags |= FIEMAP_EXTENT_ENCODED;
> + if (flags & (FIEMAP_EXTENT_DATA_TAIL | FIEMAP_EXTENT_DATA_INLINE))
> + flags |= FIEMAP_EXTENT_NOT_ALIGNED;
> +
> + extent->fe_logical = logical;
> + extent->fe_physical = phys;
> + extent->fe_length = len;
> + extent->fe_flags = flags;
> +
> + fieinfo->fi_extents_mapped++;
> +
> + if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max)
> + return 1;
> +
> +out:
> + if (flags & FIEMAP_EXTENT_LAST)
> + return 1;
> + return 0;
> +}
> +
> +static int bmap_fiemap(struct inode *inode, sector_t *block)
> +{
> + struct fiemap_extent_info fieinfo = { 0, };
> + struct fiemap_extent fextent;
> + u64 start = *block << inode->i_blkbits;
> + int error = -EINVAL;
> +
> + fextent.fe_logical = 0;
> + fextent.fe_physical = 0;
> + fieinfo.fi_extents_max = 1;
> + fieinfo.fi_extents_mapped = 0;
> + fieinfo.fi_cb_data = &fextent;
> + fieinfo.fi_start = start;
> + fieinfo.fi_len = 1 << inode->i_blkbits;
> + fieinfo.fi_cb = fiemap_fill_kernel_extent;
> + fieinfo.fi_flags = (FIEMAP_KERNEL_FIBMAP | FIEMAP_FLAG_SYNC);
> +
> + error = inode->i_op->fiemap(inode, &fieinfo);
> +
> + if (error)
> + return error;
> +
> + if (fieinfo.fi_flags & (FIEMAP_EXTENT_UNKNOWN |
> + FIEMAP_EXTENT_ENCODED |
> + FIEMAP_EXTENT_DATA_INLINE |
> + FIEMAP_EXTENT_UNWRITTEN |
> + FIEMAP_EXTENT_SHARED))
> + return -EINVAL;
> +
> + *block = (fextent.fe_physical +
> + (start - fextent.fe_logical)) >> inode->i_blkbits;
> +
> + return error;
> +}
> +
> /**
> * bmap - find a block number in a file
> * @inode: inode owning the block number being requested
> @@ -1591,10 +1663,15 @@ EXPORT_SYMBOL(iput);
> */
> int bmap(struct inode *inode, sector_t *block)
> {
> - if (!inode->i_mapping->a_ops->bmap)
> + if (inode->i_op->fiemap)
> + return bmap_fiemap(inode, block);
> +
> + if (inode->i_mapping->a_ops->bmap)
> + *block = inode->i_mapping->a_ops->bmap(inode->i_mapping,
> + *block);
> + else
> return -EINVAL;
>
> - *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
> return 0;
> }
> EXPORT_SYMBOL(bmap);
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index d72696c222de..0759ac6e4c7e 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -77,11 +77,8 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> return error;
> }
>
> -#define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC)
> -#define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED)
> -#define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE)
> -int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> - u64 phys, u64 len, u32 flags)
> +static int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo,
> + u64 logical, u64 phys, u64 len, u32 flags)
> {
> struct fiemap_extent extent;
> struct fiemap_extent __user *dest = fieinfo->fi_cb_data;
> @@ -89,17 +86,17 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> /* only count the extents */
> if (fieinfo->fi_extents_max == 0) {
> fieinfo->fi_extents_mapped++;
> - return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> + goto out;
> }
>
> if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
> return 1;
>
> - if (flags & SET_UNKNOWN_FLAGS)
> + if (flags & FIEMAP_EXTENT_DELALLOC)
> flags |= FIEMAP_EXTENT_UNKNOWN;
> - if (flags & SET_NO_UNMOUNTED_IO_FLAGS)
> + if (flags & FIEMAP_EXTENT_DATA_ENCRYPTED)
> flags |= FIEMAP_EXTENT_ENCODED;
> - if (flags & SET_NOT_ALIGNED_FLAGS)
It's too bad that we lose the "not aligned" semantic meaning here.
> + if (flags & (FIEMAP_EXTENT_DATA_TAIL | FIEMAP_EXTENT_DATA_INLINE))
> flags |= FIEMAP_EXTENT_NOT_ALIGNED;
Why doesn't this function just call fiemap_fill_kernel_extent to fill
out the onstack @extent structure? We've now implemented "fill out out
a struct fiemap_extent" twice.
>
> memset(&extent, 0, sizeof(extent));
> @@ -115,7 +112,11 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> fieinfo->fi_extents_mapped++;
> if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max)
> return 1;
> - return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> +
> +out:
> + if (flags & FIEMAP_EXTENT_LAST)
> + return 1;
> + return 0;
> }
>
> /**
> @@ -151,13 +152,23 @@ EXPORT_SYMBOL(fiemap_fill_next_extent);
> * flags, the invalid values will be written into the fieinfo structure, and
> * -EBADR is returned, which tells ioctl_fiemap() to return those values to
> * userspace. For this reason, a return code of -EBADR should be preserved.
> + * In case ->fiemap is being used for FIBMAP calls, and the filesystem does not
> + * support it, return -EINVAL.
> *
> - * Returns 0 on success, -EBADR on bad flags.
> + * Returns 0 on success, -EBADR on bad flags, -EINVAL for an unsupported FIBMAP
> + * request.
> */
> int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags)
> {
> u32 incompat_flags;
>
> + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) {
> + if (fs_flags & FIEMAP_KERNEL_FIBMAP)
> + return 0;
> +
> + return -EINVAL;
> + }
> +
> incompat_flags = fieinfo->fi_flags & ~(FIEMAP_FLAGS_COMPAT & fs_flags);
> if (incompat_flags) {
> fieinfo->fi_flags = incompat_flags;
> @@ -208,6 +219,10 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
> if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
> return -EINVAL;
>
> + /* Userspace has no access to this flag */
> + if (fiemap.fm_flags & FIEMAP_KERNEL_FIBMAP)
> + return -EINVAL;
> +
> error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
> &len);
> if (error)
> @@ -318,7 +333,8 @@ int __generic_block_fiemap(struct inode *inode,
> bool past_eof = false, whole_file = false;
> int ret = 0;
>
> - ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
> + ret = fiemap_check_flags(fieinfo,
> + FIEMAP_FLAG_SYNC | FIEMAP_KERNEL_FIBMAP);
> if (ret)
> return ret;
>
> diff --git a/fs/iomap.c b/fs/iomap.c
> index b1e88722e10b..2b182abd18e8 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -1195,7 +1195,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> ctx.fi = fi;
> ctx.prev.type = IOMAP_HOLE;
>
> - ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC);
> + ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC | FIEMAP_KERNEL_FIBMAP);
> if (ret)
> return ret;
>
> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
> index e01fd38ea935..2884395f3972 100644
> --- a/fs/ocfs2/extent_map.c
> +++ b/fs/ocfs2/extent_map.c
> @@ -747,7 +747,7 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh,
> return 0;
> }
>
> -#define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
> +#define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC | FIEMAP_KERNEL_FIBMAP)
>
> int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> @@ -756,6 +756,7 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> unsigned int hole_size;
> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> u64 len_bytes, phys_bytes, virt_bytes;
> +
> struct buffer_head *di_bh = NULL;
> struct ocfs2_extent_rec rec;
> u64 map_start = fieinfo->fi_start;
> @@ -765,6 +766,11 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> if (ret)
> return ret;
>
> + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) {
> + if (ocfs2_is_refcount_inode(inode))
> + return -EINVAL;
> + }
> +
> ret = ocfs2_inode_lock(inode, &di_bh, 0);
> if (ret) {
> mlog_errno(ret);
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b485190b7ecd..18a798e9076b 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1113,6 +1113,11 @@ xfs_vn_fiemap(
> struct fiemap_extent_info *fieinfo)
> {
> int error;
> + struct xfs_inode *ip = XFS_I(inode);
Would you mind fixing the indentation to match usual xfs style?
> +
> + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> + if (xfs_is_reflink_inode(ip) || XFS_IS_REALTIME_INODE(ip))
> + return -EINVAL;
The xfs part looks ok to me.
--D
>
> xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
> if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index a8bd3c4f6d86..233e12ccb6d3 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1709,6 +1709,10 @@ extern bool may_open_dev(const struct path *path);
> typedef int (*fiemap_fill_cb)(struct fiemap_extent_info *fieinfo, u64 logical,
> u64 phys, u64 len, u32 flags);
>
> +#define FIEMAP_KERNEL_FIBMAP 0x10000000 /* FIBMAP call through FIEMAP
> + interface. This is a kernel
> + only flag */
> +
> struct fiemap_extent_info {
> unsigned int fi_flags; /* Flags as passed from user */
> u64 fi_start;
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/9] iomap: Remove length and start fields from iomap_fiemap
2019-07-31 14:12 ` [PATCH 6/9] iomap: Remove length and start fields from iomap_fiemap Carlos Maiolino
@ 2019-07-31 23:24 ` Darrick J. Wong
0 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2019-07-31 23:24 UTC (permalink / raw)
To: Carlos Maiolino
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Wed, Jul 31, 2019 at 04:12:42PM +0200, Carlos Maiolino wrote:
> fiemap_extent_info now embeds start and length parameters, users of
> iomap_fiemap() doesn't need to pass it individually anymore.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/gfs2/inode.c | 4 +---
> fs/iomap.c | 4 +++-
> fs/xfs/xfs_iops.c | 8 ++------
> include/linux/iomap.h | 2 +-
> 4 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 5e84d5963506..df31bd8ecf6f 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -2006,8 +2006,6 @@ static int gfs2_getattr(const struct path *path, struct kstat *stat,
>
> static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> - u64 start = fieinfo->fi_start;
> - u64 len = fieinfo->fi_len;
> struct gfs2_inode *ip = GFS2_I(inode);
> struct gfs2_holder gh;
> int ret;
> @@ -2018,7 +2016,7 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> if (ret)
> goto out;
>
> - ret = iomap_fiemap(inode, fieinfo, start, len, &gfs2_iomap_ops);
> + ret = iomap_fiemap(inode, fieinfo, &gfs2_iomap_ops);
>
> gfs2_glock_dq_uninit(&gh);
>
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 97cb9d486a7d..b1e88722e10b 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -1184,9 +1184,11 @@ iomap_fiemap_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
> }
>
> int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> - loff_t start, loff_t len, const struct iomap_ops *ops)
> + const struct iomap_ops *ops)
> {
> struct fiemap_ctx ctx;
> + loff_t start = fi->fi_start;
> + loff_t len = fi->fi_len;
> loff_t ret;
>
> memset(&ctx, 0, sizeof(ctx));
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 1f4354fa989b..b485190b7ecd 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1112,18 +1112,14 @@ xfs_vn_fiemap(
> struct inode *inode,
> struct fiemap_extent_info *fieinfo)
> {
> - u64 start = fieinfo->fi_start;
> - u64 length = fieinfo->fi_len;
> int error;
>
> xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
> if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
> fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;
> - error = iomap_fiemap(inode, fieinfo, start, length,
> - &xfs_xattr_iomap_ops);
> + error = iomap_fiemap(inode, fieinfo, &xfs_xattr_iomap_ops);
> } else {
> - error = iomap_fiemap(inode, fieinfo, start, length,
> - &xfs_iomap_ops);
> + error = iomap_fiemap(inode, fieinfo, &xfs_iomap_ops);
> }
> xfs_iunlock(XFS_I(inode), XFS_IOLOCK_SHARED);
>
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 0fefb5455bda..bc4c421ee822 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -145,7 +145,7 @@ int iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
> vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf,
> const struct iomap_ops *ops);
> int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - loff_t start, loff_t len, const struct iomap_ops *ops);
> + const struct iomap_ops *ops);
> loff_t iomap_seek_hole(struct inode *inode, loff_t offset,
> const struct iomap_ops *ops);
> loff_t iomap_seek_data(struct inode *inode, loff_t offset,
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 7/9] fiemap: Use a callback to fill fiemap extents
2019-07-31 14:12 ` [PATCH 7/9] fiemap: Use a callback to fill fiemap extents Carlos Maiolino
@ 2019-07-31 23:26 ` Darrick J. Wong
0 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2019-07-31 23:26 UTC (permalink / raw)
To: Carlos Maiolino
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Wed, Jul 31, 2019 at 04:12:43PM +0200, Carlos Maiolino wrote:
> As a goal to enable fiemap infrastructure to be used by fibmap too, we need a
> way to use different helpers to fill extent data, depending on its usage. One
> helper to fill extent data stored in user address space (used in fiemap), and
> another fo fill extent data stored in kernel address space (will be used in
> fibmap).
>
> This patch sets up the usage of a callback to be used to fill in the extents.
> It transforms the current fiemap_fill_next_extent, into a simple helper to call
> the callback, avoiding unneeded changes on any filesystem, and reutilizes the
> original function as the callback used by FIEMAP.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
>
> Changelog:
>
> V3:
> - Rebase to current linux-next
> - Fix conflict on rebase
> - Merge this patch into patch 07 from V2
> - Rename fi_extents_start to fi_cb_data
>
> V2:
> - Now based on the rework on fiemap_extent_info (previous was
> based on fiemap_ctx)
>
> fs/ioctl.c | 45 ++++++++++++++++++++++++++-------------------
> include/linux/fs.h | 12 +++++++++---
> 2 files changed, 35 insertions(+), 22 deletions(-)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index ad8edcb10dc9..d72696c222de 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -77,29 +77,14 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> return error;
> }
>
> -/**
> - * fiemap_fill_next_extent - Fiemap helper function
> - * @fieinfo: Fiemap context passed into ->fiemap
> - * @logical: Extent logical start offset, in bytes
> - * @phys: Extent physical start offset, in bytes
> - * @len: Extent length, in bytes
> - * @flags: FIEMAP_EXTENT flags that describe this extent
> - *
> - * Called from file system ->fiemap callback. Will populate extent
> - * info as passed in via arguments and copy to user memory. On
> - * success, extent count on fieinfo is incremented.
> - *
> - * Returns 0 on success, -errno on error, 1 if this was the last
> - * extent that will fit in user array.
> - */
> #define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC)
> #define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED)
> #define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE)
> -int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> +int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> u64 phys, u64 len, u32 flags)
> {
> struct fiemap_extent extent;
> - struct fiemap_extent __user *dest = fieinfo->fi_extents_start;
> + struct fiemap_extent __user *dest = fieinfo->fi_cb_data;
>
> /* only count the extents */
> if (fieinfo->fi_extents_max == 0) {
> @@ -132,6 +117,27 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> return 1;
> return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> }
> +
> +/**
> + * fiemap_fill_next_extent - Fiemap helper function
> + * @fieinfo: Fiemap context passed into ->fiemap
> + * @logical: Extent logical start offset, in bytes
> + * @phys: Extent physical start offset, in bytes
> + * @len: Extent length, in bytes
> + * @flags: FIEMAP_EXTENT flags that describe this extent
> + *
> + * Called from file system ->fiemap callback. Will populate extent
> + * info as passed in via arguments and copy to user memory. On
> + * success, extent count on fieinfo is incremented.
> + *
> + * Returns 0 on success, -errno on error, 1 if this was the last
> + * extent that will fit in user array.
> + */
> +int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> + u64 phys, u64 len, u32 flags)
> +{
> + return fieinfo->fi_cb(fieinfo, logical, phys, len, flags);
> +}
> EXPORT_SYMBOL(fiemap_fill_next_extent);
>
> /**
> @@ -209,12 +215,13 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
>
> fieinfo.fi_flags = fiemap.fm_flags;
> fieinfo.fi_extents_max = fiemap.fm_extent_count;
> - fieinfo.fi_extents_start = ufiemap->fm_extents;
> + fieinfo.fi_cb_data = ufiemap->fm_extents;
> fieinfo.fi_start = fiemap.fm_start;
> fieinfo.fi_len = len;
> + fieinfo.fi_cb = fiemap_fill_user_extent;
>
> if (fiemap.fm_extent_count != 0 &&
> - !access_ok(fieinfo.fi_extents_start,
> + !access_ok(fieinfo.fi_cb_data,
> fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
> return -EFAULT;
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7b744b7de24e..a8bd3c4f6d86 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -66,6 +66,7 @@ struct fscrypt_info;
> struct fscrypt_operations;
> struct fs_context;
> struct fs_parameter_description;
> +struct fiemap_extent_info;
>
> extern void __init inode_init(void);
> extern void __init inode_init_early(void);
> @@ -1704,16 +1705,21 @@ extern bool may_open_dev(const struct path *path);
> /*
> * VFS FS_IOC_FIEMAP helper definitions.
> */
> +
> +typedef int (*fiemap_fill_cb)(struct fiemap_extent_info *fieinfo, u64 logical,
> + u64 phys, u64 len, u32 flags);
> +
> struct fiemap_extent_info {
> unsigned int fi_flags; /* Flags as passed from user */
> u64 fi_start;
> u64 fi_len;
> unsigned int fi_extents_mapped; /* Number of mapped extents */
> unsigned int fi_extents_max; /* Size of fiemap_extent array */
> - struct fiemap_extent __user *fi_extents_start; /* Start of
> - fiemap_extent
> - array */
> + void *fi_cb_data; /* Start of fiemap_extent
> + array */
> + fiemap_fill_cb fi_cb;
> };
> +
> int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> u64 phys, u64 len, u32 flags);
> int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info
2019-07-31 14:12 ` [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info Carlos Maiolino
@ 2019-07-31 23:28 ` Darrick J. Wong
2019-08-02 9:51 ` Carlos Maiolino
0 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2019-07-31 23:28 UTC (permalink / raw)
To: Carlos Maiolino
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Wed, Jul 31, 2019 at 04:12:41PM +0200, Carlos Maiolino wrote:
> As the overall goal to deprecate fibmap, Christoph suggested a rework of
> the ->fiemap API, in a way we could pass to it a callback to fill the
> fiemap structure (one of these callbacks being fiemap_fill_next_extent).
>
> To avoid the need to add several fields into the ->fiemap method, aggregate
> everything into a single data structure, and pass it along.
>
> This patch isn't suppose to add any functional change, only to update
> filesystems providing ->fiemap() method.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> fs/bad_inode.c | 3 +--
> fs/btrfs/inode.c | 5 +++--
> fs/ext2/ext2.h | 3 +--
> fs/ext2/inode.c | 6 ++----
> fs/ext4/ext4.h | 3 +--
> fs/ext4/extents.c | 8 ++++----
> fs/f2fs/data.c | 5 +++--
> fs/f2fs/f2fs.h | 3 +--
> fs/gfs2/inode.c | 5 +++--
> fs/hpfs/file.c | 4 ++--
> fs/ioctl.c | 16 ++++++++++------
> fs/nilfs2/inode.c | 5 +++--
> fs/nilfs2/nilfs.h | 3 +--
> fs/ocfs2/extent_map.c | 5 +++--
> fs/ocfs2/extent_map.h | 3 +--
> fs/overlayfs/inode.c | 5 ++---
> fs/xfs/xfs_iops.c | 10 +++++-----
> include/linux/fs.h | 21 +++++++++++----------
> 18 files changed, 57 insertions(+), 56 deletions(-)
>
> diff --git a/fs/bad_inode.c b/fs/bad_inode.c
> index 8035d2a44561..21dfaf876814 100644
> --- a/fs/bad_inode.c
> +++ b/fs/bad_inode.c
> @@ -120,8 +120,7 @@ static struct posix_acl *bad_inode_get_acl(struct inode *inode, int type)
> }
>
> static int bad_inode_fiemap(struct inode *inode,
> - struct fiemap_extent_info *fieinfo, u64 start,
> - u64 len)
> + struct fiemap_extent_info *fieinfo)
> {
> return -EIO;
> }
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 82fdda8ff5ab..caa06a8ac767 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8600,9 +8600,10 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>
> #define BTRFS_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
>
> -static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - __u64 start, __u64 len)
> +static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> + u64 start = fieinfo->fi_start;
> + u64 len = fieinfo->fi_len;
> int ret;
>
> ret = fiemap_check_flags(fieinfo, BTRFS_FIEMAP_FLAGS);
> diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
> index 10ab238de9a6..284df1af9474 100644
> --- a/fs/ext2/ext2.h
> +++ b/fs/ext2/ext2.h
> @@ -760,8 +760,7 @@ extern int ext2_get_block(struct inode *, sector_t, struct buffer_head *, int);
> extern int ext2_setattr (struct dentry *, struct iattr *);
> extern int ext2_getattr (const struct path *, struct kstat *, u32, unsigned int);
> extern void ext2_set_inode_flags(struct inode *inode);
> -extern int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - u64 start, u64 len);
> +extern int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo);
>
> /* ioctl.c */
> extern long ext2_ioctl(struct file *, unsigned int, unsigned long);
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index c27c27300d95..267392335f38 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -855,11 +855,9 @@ const struct iomap_ops ext2_iomap_ops = {
> const struct iomap_ops ext2_iomap_ops;
> #endif /* CONFIG_FS_DAX */
>
> -int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - u64 start, u64 len)
> +int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> - return generic_block_fiemap(inode, fieinfo, start, len,
> - ext2_get_block);
> + return generic_block_fiemap(inode, fieinfo, ext2_get_block);
> }
>
> static int ext2_writepage(struct page *page, struct writeback_control *wbc)
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 82ffdacdc7fa..e4cb40b5893b 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3154,8 +3154,7 @@ extern struct ext4_ext_path *ext4_find_extent(struct inode *, ext4_lblk_t,
> extern void ext4_ext_drop_refs(struct ext4_ext_path *);
> extern int ext4_ext_check_inode(struct inode *inode);
> extern ext4_lblk_t ext4_ext_next_allocated_block(struct ext4_ext_path *path);
> -extern int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - __u64 start, __u64 len);
> +extern int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo);
> extern int ext4_ext_precache(struct inode *inode);
> extern int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len);
> extern int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len);
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 0f89f5190cd7..436e564ebdd6 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -5038,9 +5038,10 @@ static int ext4_xattr_fiemap(struct inode *inode,
> return (error < 0 ? error : 0);
> }
>
> -int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - __u64 start, __u64 len)
> +int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> + u64 start = fieinfo->fi_start;
> + u64 len = fieinfo->fi_len;
> ext4_lblk_t start_blk;
> int error = 0;
>
> @@ -5062,8 +5063,7 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>
> /* fallback to generic here if not in extents fmt */
> if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
> - return generic_block_fiemap(inode, fieinfo, start, len,
> - ext4_get_block);
> + return generic_block_fiemap(inode, fieinfo, ext4_get_block);
>
> if (fiemap_check_flags(fieinfo, EXT4_FIEMAP_FLAGS))
> return -EBADR;
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 9727944139f2..2979ca40d192 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1409,9 +1409,10 @@ static int f2fs_xattr_fiemap(struct inode *inode,
> return (err < 0 ? err : 0);
> }
>
> -int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - u64 start, u64 len)
> +int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> + u64 start = fieinfo->fi_start;
> + u64 len = fieinfo->fi_len;
> struct buffer_head map_bh;
> sector_t start_blk, last_blk;
> pgoff_t next_pgofs;
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 87f75ebd2fd6..fb33809c2552 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3155,8 +3155,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio);
> void __do_map_lock(struct f2fs_sb_info *sbi, int flag, bool lock);
> int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
> int create, int flag);
> -int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - u64 start, u64 len);
> +int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo);
> bool f2fs_should_update_inplace(struct inode *inode, struct f2fs_io_info *fio);
> bool f2fs_should_update_outplace(struct inode *inode, struct f2fs_io_info *fio);
> void f2fs_invalidate_page(struct page *page, unsigned int offset,
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 998051c4aea7..5e84d5963506 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -2004,9 +2004,10 @@ static int gfs2_getattr(const struct path *path, struct kstat *stat,
> return 0;
> }
>
> -static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - u64 start, u64 len)
> +static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> + u64 start = fieinfo->fi_start;
> + u64 len = fieinfo->fi_len;
> struct gfs2_inode *ip = GFS2_I(inode);
> struct gfs2_holder gh;
> int ret;
> diff --git a/fs/hpfs/file.c b/fs/hpfs/file.c
> index 1ecec124e76f..0eece4ae1f11 100644
> --- a/fs/hpfs/file.c
> +++ b/fs/hpfs/file.c
> @@ -190,9 +190,9 @@ static sector_t _hpfs_bmap(struct address_space *mapping, sector_t block)
> return generic_block_bmap(mapping, block, hpfs_get_block);
> }
>
> -static int hpfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, u64 start, u64 len)
> +static int hpfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> - return generic_block_fiemap(inode, fieinfo, start, len, hpfs_get_block);
> + return generic_block_fiemap(inode, fieinfo, hpfs_get_block);
> }
>
> const struct address_space_operations hpfs_aops = {
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 6b589c873bc2..ad8edcb10dc9 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -210,6 +210,8 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
> fieinfo.fi_flags = fiemap.fm_flags;
> fieinfo.fi_extents_max = fiemap.fm_extent_count;
> fieinfo.fi_extents_start = ufiemap->fm_extents;
> + fieinfo.fi_start = fiemap.fm_start;
> + fieinfo.fi_len = len;
>
> if (fiemap.fm_extent_count != 0 &&
> !access_ok(fieinfo.fi_extents_start,
> @@ -219,7 +221,7 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
> if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
> filemap_write_and_wait(inode->i_mapping);
>
> - error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start, len);
> + error = inode->i_op->fiemap(inode, &fieinfo);
> fiemap.fm_flags = fieinfo.fi_flags;
> fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
> if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
> @@ -296,9 +298,11 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
> */
>
> int __generic_block_fiemap(struct inode *inode,
> - struct fiemap_extent_info *fieinfo, loff_t start,
> - loff_t len, get_block_t *get_block)
> + struct fiemap_extent_info *fieinfo,
> + get_block_t *get_block)
> {
> + loff_t start = fieinfo->fi_start;
> + loff_t len = fieinfo->fi_len;
> struct buffer_head map_bh;
> sector_t start_blk, last_blk;
> loff_t isize = i_size_read(inode);
> @@ -455,12 +459,12 @@ EXPORT_SYMBOL(__generic_block_fiemap);
> */
>
> int generic_block_fiemap(struct inode *inode,
> - struct fiemap_extent_info *fieinfo, u64 start,
> - u64 len, get_block_t *get_block)
> + struct fiemap_extent_info *fieinfo,
> + get_block_t *get_block)
> {
> int ret;
> inode_lock(inode);
> - ret = __generic_block_fiemap(inode, fieinfo, start, len, get_block);
> + ret = __generic_block_fiemap(inode, fieinfo, get_block);
> inode_unlock(inode);
> return ret;
> }
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 671085512e0f..1f37d086371c 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -992,9 +992,10 @@ void nilfs_dirty_inode(struct inode *inode, int flags)
> nilfs_transaction_commit(inode->i_sb); /* never fails */
> }
>
> -int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - __u64 start, __u64 len)
> +int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> + u64 start = fieinfo->fi_start;
> + u64 len = fieinfo->fi_len;
> struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
> __u64 logical = 0, phys = 0, size = 0;
> __u32 flags = 0;
> diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h
> index a2f247b6a209..55d1307ed710 100644
> --- a/fs/nilfs2/nilfs.h
> +++ b/fs/nilfs2/nilfs.h
> @@ -276,8 +276,7 @@ extern int nilfs_inode_dirty(struct inode *);
> int nilfs_set_file_dirty(struct inode *inode, unsigned int nr_dirty);
> extern int __nilfs_mark_inode_dirty(struct inode *, int);
> extern void nilfs_dirty_inode(struct inode *, int flags);
> -int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - __u64 start, __u64 len);
> +int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo);
> static inline int nilfs_mark_inode_dirty(struct inode *inode)
> {
> return __nilfs_mark_inode_dirty(inode, I_DIRTY);
> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
> index 06cb96462bf9..e01fd38ea935 100644
> --- a/fs/ocfs2/extent_map.c
> +++ b/fs/ocfs2/extent_map.c
> @@ -749,8 +749,7 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh,
>
> #define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
>
> -int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - u64 map_start, u64 map_len)
> +int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> int ret, is_last;
> u32 mapping_end, cpos;
> @@ -759,6 +758,8 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> u64 len_bytes, phys_bytes, virt_bytes;
> struct buffer_head *di_bh = NULL;
> struct ocfs2_extent_rec rec;
> + u64 map_start = fieinfo->fi_start;
> + u64 map_len = fieinfo->fi_len;
>
> ret = fiemap_check_flags(fieinfo, OCFS2_FIEMAP_FLAGS);
> if (ret)
> diff --git a/fs/ocfs2/extent_map.h b/fs/ocfs2/extent_map.h
> index 1057586ec19f..793be96099c0 100644
> --- a/fs/ocfs2/extent_map.h
> +++ b/fs/ocfs2/extent_map.h
> @@ -50,8 +50,7 @@ int ocfs2_get_clusters(struct inode *inode, u32 v_cluster, u32 *p_cluster,
> int ocfs2_extent_map_get_blocks(struct inode *inode, u64 v_blkno, u64 *p_blkno,
> u64 *ret_count, unsigned int *extent_flags);
>
> -int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - u64 map_start, u64 map_len);
> +int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo);
>
> int ocfs2_overwrite_io(struct inode *inode, struct buffer_head *di_bh,
> u64 map_start, u64 map_len);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 3b7ed5d2279c..6f00b0ef6b43 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -456,8 +456,7 @@ int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags)
> return 0;
> }
>
> -static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - u64 start, u64 len)
> +static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> {
> int err;
> struct inode *realinode = ovl_inode_real(inode);
> @@ -471,7 +470,7 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
> filemap_write_and_wait(realinode->i_mapping);
>
> - err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
> + err = realinode->i_op->fiemap(realinode, fieinfo);
> revert_creds(old_cred);
>
> return err;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 74047bd0c1ae..1f4354fa989b 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1109,12 +1109,12 @@ xfs_vn_update_time(
>
> STATIC int
> xfs_vn_fiemap(
> - struct inode *inode,
> - struct fiemap_extent_info *fieinfo,
> - u64 start,
> - u64 length)
> + struct inode *inode,
> + struct fiemap_extent_info *fieinfo)
> {
> - int error;
> + u64 start = fieinfo->fi_start;
> + u64 length = fieinfo->fi_len;
> + int error;
Would be nice if the variable name indentation was consistent here, but
otherwise the xfs part looks ok.
>
> xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
> if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index d5e7c744aea6..7b744b7de24e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1705,11 +1705,14 @@ extern bool may_open_dev(const struct path *path);
> * VFS FS_IOC_FIEMAP helper definitions.
> */
> struct fiemap_extent_info {
> - unsigned int fi_flags; /* Flags as passed from user */
> - unsigned int fi_extents_mapped; /* Number of mapped extents */
> - unsigned int fi_extents_max; /* Size of fiemap_extent array */
> - struct fiemap_extent __user *fi_extents_start; /* Start of
> - fiemap_extent array */
> + unsigned int fi_flags; /* Flags as passed from user */
> + u64 fi_start;
> + u64 fi_len;
Comments for these two new fields?
--D
> + unsigned int fi_extents_mapped; /* Number of mapped extents */
> + unsigned int fi_extents_max; /* Size of fiemap_extent array */
> + struct fiemap_extent __user *fi_extents_start; /* Start of
> + fiemap_extent
> + array */
> };
> int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> u64 phys, u64 len, u32 flags);
> @@ -1841,8 +1844,7 @@ struct inode_operations {
> int (*setattr) (struct dentry *, struct iattr *);
> int (*getattr) (const struct path *, struct kstat *, u32, unsigned int);
> ssize_t (*listxattr) (struct dentry *, char *, size_t);
> - int (*fiemap)(struct inode *, struct fiemap_extent_info *, u64 start,
> - u64 len);
> + int (*fiemap)(struct inode *, struct fiemap_extent_info *);
> int (*update_time)(struct inode *, struct timespec64 *, int);
> int (*atomic_open)(struct inode *, struct dentry *,
> struct file *, unsigned open_flag,
> @@ -3199,11 +3201,10 @@ extern int vfs_readlink(struct dentry *, char __user *, int);
>
> extern int __generic_block_fiemap(struct inode *inode,
> struct fiemap_extent_info *fieinfo,
> - loff_t start, loff_t len,
> get_block_t *get_block);
> extern int generic_block_fiemap(struct inode *inode,
> - struct fiemap_extent_info *fieinfo, u64 start,
> - u64 len, get_block_t *get_block);
> + struct fiemap_extent_info *fieinfo,
> + get_block_t *get_block);
>
> extern struct file_system_type *get_filesystem(struct file_system_type *fs);
> extern void put_filesystem(struct file_system_type *fs);
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 9/9] xfs: Get rid of ->bmap
2019-07-31 14:12 ` [PATCH 9/9] xfs: Get rid of ->bmap Carlos Maiolino
@ 2019-07-31 23:30 ` Darrick J. Wong
0 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2019-07-31 23:30 UTC (permalink / raw)
To: Carlos Maiolino
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Wed, Jul 31, 2019 at 04:12:45PM +0200, Carlos Maiolino wrote:
> We don't need ->bmap anymore, only usage for it was FIBMAP, which is now
> gone.
>
> Also kill iomap_bmap() and iomap_bmap_actor once it has no users anymore.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>
> Changelog:
>
> V2:
> - Kill iomap_bmap() and iomap_bmap_actor()
>
> fs/iomap.c | 34 ----------------------------------
> fs/xfs/xfs_aops.c | 24 ------------------------
> fs/xfs/xfs_trace.h | 1 -
> 3 files changed, 59 deletions(-)
>
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 2b182abd18e8..12e6a575feb4 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
Needs rebasing against 5.3-rc1.
> @@ -2153,37 +2153,3 @@ int iomap_swapfile_activate(struct swap_info_struct *sis,
> }
> EXPORT_SYMBOL_GPL(iomap_swapfile_activate);
> #endif /* CONFIG_SWAP */
> -
> -static loff_t
> -iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,
> - void *data, struct iomap *iomap)
> -{
> - sector_t *bno = data, addr;
> -
> - if (iomap->type == IOMAP_MAPPED) {
> - addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits;
> - if (addr > INT_MAX)
> - WARN(1, "would truncate bmap result\n");
> - else
> - *bno = addr;
> - }
> - return 0;
> -}
> -
> -/* legacy ->bmap interface. 0 is the error return (!) */
> -sector_t
> -iomap_bmap(struct address_space *mapping, sector_t bno,
> - const struct iomap_ops *ops)
> -{
> - struct inode *inode = mapping->host;
> - loff_t pos = bno << inode->i_blkbits;
> - unsigned blocksize = i_blocksize(inode);
> -
> - if (filemap_write_and_wait(mapping))
> - return 0;
> -
> - bno = 0;
> - iomap_apply(inode, pos, blocksize, 0, ops, &bno, iomap_bmap_actor);
> - return bno;
> -}
> -EXPORT_SYMBOL_GPL(iomap_bmap);
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 3619e9e8d359..76ee495eba1a 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -1006,29 +1006,6 @@ xfs_vm_releasepage(
> return iomap_releasepage(page, gfp_mask);
> }
>
> -STATIC sector_t
> -xfs_vm_bmap(
> - struct address_space *mapping,
> - sector_t block)
> -{
> - struct xfs_inode *ip = XFS_I(mapping->host);
> -
> - trace_xfs_vm_bmap(ip);
> -
> - /*
> - * The swap code (ab-)uses ->bmap to get a block mapping and then
> - * bypasses the file system for actual I/O. We really can't allow
> - * that on reflinks inodes, so we have to skip out here. And yes,
> - * 0 is the magic code for a bmap error.
> - *
> - * Since we don't pass back blockdev info, we can't return bmap
> - * information for rt files either.
> - */
> - if (xfs_is_cow_inode(ip) || XFS_IS_REALTIME_INODE(ip))
Wait, this isn't the same check that was added to the xfs fiemap
implementation.
--D
> - return 0;
> - return iomap_bmap(mapping, block, &xfs_iomap_ops);
> -}
> -
> STATIC int
> xfs_vm_readpage(
> struct file *unused,
> @@ -1067,7 +1044,6 @@ const struct address_space_operations xfs_address_space_operations = {
> .set_page_dirty = iomap_set_page_dirty,
> .releasepage = xfs_vm_releasepage,
> .invalidatepage = xfs_vm_invalidatepage,
> - .bmap = xfs_vm_bmap,
> .direct_IO = noop_direct_IO,
> .migratepage = iomap_migrate_page,
> .is_partially_uptodate = iomap_is_partially_uptodate,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 47fb07d86efd..3a45a3971dce 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -621,7 +621,6 @@ DEFINE_INODE_EVENT(xfs_readdir);
> #ifdef CONFIG_XFS_POSIX_ACL
> DEFINE_INODE_EVENT(xfs_get_acl);
> #endif
> -DEFINE_INODE_EVENT(xfs_vm_bmap);
> DEFINE_INODE_EVENT(xfs_file_ioctl);
> DEFINE_INODE_EVENT(xfs_file_compat_ioctl);
> DEFINE_INODE_EVENT(xfs_ioctl_setattr);
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] Use FIEMAP for FIBMAP calls
2019-07-31 23:22 ` Darrick J. Wong
@ 2019-07-31 23:31 ` Darrick J. Wong
2019-08-02 13:52 ` Carlos Maiolino
2019-08-06 5:41 ` Christoph Hellwig
2019-08-02 13:48 ` Carlos Maiolino
1 sibling, 2 replies; 47+ messages in thread
From: Darrick J. Wong @ 2019-07-31 23:31 UTC (permalink / raw)
To: Carlos Maiolino
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Wed, Jul 31, 2019 at 04:22:54PM -0700, Darrick J. Wong wrote:
> On Wed, Jul 31, 2019 at 04:12:44PM +0200, Carlos Maiolino wrote:
> > Enables the usage of FIEMAP ioctl infrastructure to handle FIBMAP calls.
> > From now on, ->bmap() methods can start to be removed from filesystems
> > which already provides ->fiemap().
> >
> > This adds a new helper - bmap_fiemap() - which is used to fill in the
> > fiemap request, call into the underlying filesystem and check the flags
> > set in the extent requested.
> >
> > Add a new fiemap fill extent callback to handle the in-kernel only
> > fiemap_extent structure used for FIBMAP.
> >
> > The new FIEMAP_KERNEL_FIBMAP flag, is used to tell the filesystem
> > ->fiemap interface, that the call is coming from ioctl_fibmap. The
> > addition of this new flag, requires an update to fiemap_check_flags(),
> > so it doesn't treat FIBMAP requests as invalid.
> >
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >
> > Changelog:
> >
> > V4:
> > - Fix if conditional in bmap()
> > - Add filesystem-specific modifications
> > V3:
> > - Add FIEMAP_EXTENT_SHARED to the list of invalid extents in
> > bmap_fiemap()
> > - Rename fi_extents_start to fi_cb_data
> > - Use if conditional instead of ternary operator
> > - Make fiemap_fill_* callbacks static (which required the
> > removal of some macros
> > - Set FIEMAP_FLAG_SYNC when calling in ->fiemap method from
> > fibmap
> > - Add FIEMAP_KERNEL_FIBMAP flag, to identify the usage of fiemap
> > infrastructure for fibmap calls, defined in fs.h so it's not
> > exported to userspace.
> > - Update fiemap_check_flags() to understand FIEMAP_KERNEL_FIBMAP
> > - Update filesystems supporting both FIBMAP and FIEMAP, which
> > need extra checks on FIBMAP calls
> >
> > V2:
> > - Now based on the updated fiemap_extent_info,
> > - move the fiemap call itself to a new helper
> >
> > fs/ext4/extents.c | 7 +++-
> > fs/f2fs/data.c | 10 +++++-
> > fs/gfs2/inode.c | 6 +++-
> > fs/inode.c | 81 +++++++++++++++++++++++++++++++++++++++++--
> > fs/ioctl.c | 40 ++++++++++++++-------
> > fs/iomap.c | 2 +-
> > fs/ocfs2/extent_map.c | 8 ++++-
> > fs/xfs/xfs_iops.c | 5 +++
> > include/linux/fs.h | 4 +++
> > 9 files changed, 144 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 436e564ebdd6..093b6a07067f 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -5001,7 +5001,9 @@ static int ext4_find_delayed_extent(struct inode *inode,
> > return next_del;
> > }
> > /* fiemap flags we can handle specified here */
> > -#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC|FIEMAP_FLAG_XATTR)
> > +#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC | \
> > + FIEMAP_FLAG_XATTR| \
> > + FIEMAP_KERNEL_FIBMAP)
> >
> > static int ext4_xattr_fiemap(struct inode *inode,
> > struct fiemap_extent_info *fieinfo)
> > @@ -5048,6 +5050,9 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > if (ext4_has_inline_data(inode)) {
> > int has_inline = 1;
> >
> > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> > + return -EINVAL;
>
> Wouldn't the inline data case be caught by fiemap_bmap and turned into
> -EINVAL?
>
> > +
> > error = ext4_inline_data_fiemap(inode, fieinfo, &has_inline,
> > start, len);
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 2979ca40d192..29b6c48fb6cc 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1409,6 +1409,9 @@ static int f2fs_xattr_fiemap(struct inode *inode,
> > return (err < 0 ? err : 0);
> > }
> >
> > +#define F2FS_FIEMAP_COMPAT (FIEMAP_FLAG_SYNC | \
> > + FIEMAP_FLAG_XATTR| \
> > + FIEMAP_KERNEL_FIBMAP)
> > int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > {
> > u64 start = fieinfo->fi_start;
> > @@ -1426,7 +1429,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > return ret;
> > }
> >
> > - ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
> > + ret = fiemap_check_flags(fieinfo, F2FS_FIEMAP_COMPAT);
> > if (ret)
> > return ret;
> >
> > @@ -1438,6 +1441,11 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > }
> >
> > if (f2fs_has_inline_data(inode)) {
> > +
> > + ret = -EINVAL;
> > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> > + goto out;
> > +
> > ret = f2fs_inline_data_fiemap(inode, fieinfo, start, len);
> > if (ret != -EAGAIN)
> > goto out;
> > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > index df31bd8ecf6f..30554b4f49c3 100644
> > --- a/fs/gfs2/inode.c
> > +++ b/fs/gfs2/inode.c
> > @@ -2016,7 +2016,11 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > if (ret)
> > goto out;
> >
> > - ret = iomap_fiemap(inode, fieinfo, &gfs2_iomap_ops);
> > + if (gfs2_is_stuffed(ip) &&
> > + (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP))
> > + ret = -EINVAL;
> > + else
> > + ret = iomap_fiemap(inode, fieinfo, &gfs2_iomap_ops);
> >
> > gfs2_glock_dq_uninit(&gh);
> >
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 824fa54d393d..02552b09e77f 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -1575,6 +1575,78 @@ void iput(struct inode *inode)
> > }
> > EXPORT_SYMBOL(iput);
> >
> > +static int fiemap_fill_kernel_extent(struct fiemap_extent_info *fieinfo,
> > + u64 logical, u64 phys, u64 len, u32 flags)
> > +{
> > + struct fiemap_extent *extent = fieinfo->fi_cb_data;
> > +
> > + /* only count the extents */
> > + if (fieinfo->fi_cb_data == 0) {
> > + fieinfo->fi_extents_mapped++;
> > + goto out;
> > + }
> > +
> > + if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
> > + return 1;
> > +
> > + if (flags & FIEMAP_EXTENT_DELALLOC)
> > + flags |= FIEMAP_EXTENT_UNKNOWN;
> > + if (flags & FIEMAP_EXTENT_DATA_ENCRYPTED)
> > + flags |= FIEMAP_EXTENT_ENCODED;
> > + if (flags & (FIEMAP_EXTENT_DATA_TAIL | FIEMAP_EXTENT_DATA_INLINE))
> > + flags |= FIEMAP_EXTENT_NOT_ALIGNED;
> > +
> > + extent->fe_logical = logical;
> > + extent->fe_physical = phys;
> > + extent->fe_length = len;
> > + extent->fe_flags = flags;
> > +
> > + fieinfo->fi_extents_mapped++;
> > +
> > + if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max)
> > + return 1;
> > +
> > +out:
> > + if (flags & FIEMAP_EXTENT_LAST)
> > + return 1;
> > + return 0;
> > +}
> > +
> > +static int bmap_fiemap(struct inode *inode, sector_t *block)
> > +{
> > + struct fiemap_extent_info fieinfo = { 0, };
> > + struct fiemap_extent fextent;
> > + u64 start = *block << inode->i_blkbits;
> > + int error = -EINVAL;
> > +
> > + fextent.fe_logical = 0;
> > + fextent.fe_physical = 0;
> > + fieinfo.fi_extents_max = 1;
> > + fieinfo.fi_extents_mapped = 0;
> > + fieinfo.fi_cb_data = &fextent;
> > + fieinfo.fi_start = start;
> > + fieinfo.fi_len = 1 << inode->i_blkbits;
> > + fieinfo.fi_cb = fiemap_fill_kernel_extent;
> > + fieinfo.fi_flags = (FIEMAP_KERNEL_FIBMAP | FIEMAP_FLAG_SYNC);
> > +
> > + error = inode->i_op->fiemap(inode, &fieinfo);
> > +
> > + if (error)
> > + return error;
> > +
> > + if (fieinfo.fi_flags & (FIEMAP_EXTENT_UNKNOWN |
> > + FIEMAP_EXTENT_ENCODED |
> > + FIEMAP_EXTENT_DATA_INLINE |
> > + FIEMAP_EXTENT_UNWRITTEN |
> > + FIEMAP_EXTENT_SHARED))
> > + return -EINVAL;
> > +
> > + *block = (fextent.fe_physical +
> > + (start - fextent.fe_logical)) >> inode->i_blkbits;
> > +
> > + return error;
> > +}
> > +
> > /**
> > * bmap - find a block number in a file
> > * @inode: inode owning the block number being requested
> > @@ -1591,10 +1663,15 @@ EXPORT_SYMBOL(iput);
> > */
> > int bmap(struct inode *inode, sector_t *block)
> > {
> > - if (!inode->i_mapping->a_ops->bmap)
> > + if (inode->i_op->fiemap)
> > + return bmap_fiemap(inode, block);
> > +
> > + if (inode->i_mapping->a_ops->bmap)
> > + *block = inode->i_mapping->a_ops->bmap(inode->i_mapping,
> > + *block);
> > + else
> > return -EINVAL;
> >
> > - *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
> > return 0;
> > }
> > EXPORT_SYMBOL(bmap);
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index d72696c222de..0759ac6e4c7e 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -77,11 +77,8 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> > return error;
> > }
> >
> > -#define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC)
> > -#define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED)
> > -#define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE)
> > -int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > - u64 phys, u64 len, u32 flags)
> > +static int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo,
> > + u64 logical, u64 phys, u64 len, u32 flags)
> > {
> > struct fiemap_extent extent;
> > struct fiemap_extent __user *dest = fieinfo->fi_cb_data;
> > @@ -89,17 +86,17 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > /* only count the extents */
> > if (fieinfo->fi_extents_max == 0) {
> > fieinfo->fi_extents_mapped++;
> > - return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> > + goto out;
> > }
> >
> > if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
> > return 1;
> >
> > - if (flags & SET_UNKNOWN_FLAGS)
> > + if (flags & FIEMAP_EXTENT_DELALLOC)
> > flags |= FIEMAP_EXTENT_UNKNOWN;
> > - if (flags & SET_NO_UNMOUNTED_IO_FLAGS)
> > + if (flags & FIEMAP_EXTENT_DATA_ENCRYPTED)
> > flags |= FIEMAP_EXTENT_ENCODED;
> > - if (flags & SET_NOT_ALIGNED_FLAGS)
>
> It's too bad that we lose the "not aligned" semantic meaning here.
>
> > + if (flags & (FIEMAP_EXTENT_DATA_TAIL | FIEMAP_EXTENT_DATA_INLINE))
> > flags |= FIEMAP_EXTENT_NOT_ALIGNED;
>
> Why doesn't this function just call fiemap_fill_kernel_extent to fill
> out the onstack @extent structure? We've now implemented "fill out out
> a struct fiemap_extent" twice.
>
> >
> > memset(&extent, 0, sizeof(extent));
> > @@ -115,7 +112,11 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > fieinfo->fi_extents_mapped++;
> > if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max)
> > return 1;
> > - return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> > +
> > +out:
> > + if (flags & FIEMAP_EXTENT_LAST)
> > + return 1;
> > + return 0;
> > }
> >
> > /**
> > @@ -151,13 +152,23 @@ EXPORT_SYMBOL(fiemap_fill_next_extent);
> > * flags, the invalid values will be written into the fieinfo structure, and
> > * -EBADR is returned, which tells ioctl_fiemap() to return those values to
> > * userspace. For this reason, a return code of -EBADR should be preserved.
> > + * In case ->fiemap is being used for FIBMAP calls, and the filesystem does not
> > + * support it, return -EINVAL.
> > *
> > - * Returns 0 on success, -EBADR on bad flags.
> > + * Returns 0 on success, -EBADR on bad flags, -EINVAL for an unsupported FIBMAP
> > + * request.
> > */
> > int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags)
> > {
> > u32 incompat_flags;
> >
> > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) {
> > + if (fs_flags & FIEMAP_KERNEL_FIBMAP)
> > + return 0;
> > +
> > + return -EINVAL;
> > + }
> > +
> > incompat_flags = fieinfo->fi_flags & ~(FIEMAP_FLAGS_COMPAT & fs_flags);
> > if (incompat_flags) {
> > fieinfo->fi_flags = incompat_flags;
> > @@ -208,6 +219,10 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
> > if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
> > return -EINVAL;
> >
> > + /* Userspace has no access to this flag */
> > + if (fiemap.fm_flags & FIEMAP_KERNEL_FIBMAP)
> > + return -EINVAL;
> > +
> > error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
> > &len);
> > if (error)
> > @@ -318,7 +333,8 @@ int __generic_block_fiemap(struct inode *inode,
> > bool past_eof = false, whole_file = false;
> > int ret = 0;
> >
> > - ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
> > + ret = fiemap_check_flags(fieinfo,
> > + FIEMAP_FLAG_SYNC | FIEMAP_KERNEL_FIBMAP);
> > if (ret)
> > return ret;
> >
> > diff --git a/fs/iomap.c b/fs/iomap.c
> > index b1e88722e10b..2b182abd18e8 100644
> > --- a/fs/iomap.c
> > +++ b/fs/iomap.c
> > @@ -1195,7 +1195,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> > ctx.fi = fi;
> > ctx.prev.type = IOMAP_HOLE;
> >
> > - ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC);
> > + ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC | FIEMAP_KERNEL_FIBMAP);
> > if (ret)
> > return ret;
> >
> > diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
> > index e01fd38ea935..2884395f3972 100644
> > --- a/fs/ocfs2/extent_map.c
> > +++ b/fs/ocfs2/extent_map.c
> > @@ -747,7 +747,7 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh,
> > return 0;
> > }
> >
> > -#define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
> > +#define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC | FIEMAP_KERNEL_FIBMAP)
> >
> > int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > {
> > @@ -756,6 +756,7 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > unsigned int hole_size;
> > struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> > u64 len_bytes, phys_bytes, virt_bytes;
> > +
> > struct buffer_head *di_bh = NULL;
> > struct ocfs2_extent_rec rec;
> > u64 map_start = fieinfo->fi_start;
> > @@ -765,6 +766,11 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > if (ret)
> > return ret;
> >
> > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) {
> > + if (ocfs2_is_refcount_inode(inode))
> > + return -EINVAL;
> > + }
> > +
> > ret = ocfs2_inode_lock(inode, &di_bh, 0);
> > if (ret) {
> > mlog_errno(ret);
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index b485190b7ecd..18a798e9076b 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -1113,6 +1113,11 @@ xfs_vn_fiemap(
> > struct fiemap_extent_info *fieinfo)
> > {
> > int error;
> > + struct xfs_inode *ip = XFS_I(inode);
>
> Would you mind fixing the indentation to match usual xfs style?
>
> > +
> > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> > + if (xfs_is_reflink_inode(ip) || XFS_IS_REALTIME_INODE(ip))
> > + return -EINVAL;
>
> The xfs part looks ok to me.
No it doesn't. This check ought to be xfs_is_cow_inode() to match the
code being removed in the last patch:
if ((fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) &&
(xfs_is_cow_inode(ip) || XFS_IS_REALTIME_INODE(ip)))
return -EINVAL;
> --D
>
> >
> > xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
> > if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index a8bd3c4f6d86..233e12ccb6d3 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1709,6 +1709,10 @@ extern bool may_open_dev(const struct path *path);
> > typedef int (*fiemap_fill_cb)(struct fiemap_extent_info *fieinfo, u64 logical,
> > u64 phys, u64 len, u32 flags);
> >
> > +#define FIEMAP_KERNEL_FIBMAP 0x10000000 /* FIBMAP call through FIEMAP
> > + interface. This is a kernel
> > + only flag */
> > +
> > struct fiemap_extent_info {
> > unsigned int fi_flags; /* Flags as passed from user */
> > u64 fi_start;
> > --
> > 2.20.1
> >
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-07-31 23:12 ` Darrick J. Wong
@ 2019-08-02 9:19 ` Carlos Maiolino
2019-08-02 15:14 ` Darrick J. Wong
0 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-02 9:19 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
Hi Darrick.
> > + return error;
> > +
> > + block = ur_block;
> > + error = bmap(inode, &block);
> > +
> > + if (error)
> > + ur_block = 0;
> > + else
> > + ur_block = block;
>
> What happens if ur_block > INT_MAX? Shouldn't we return zero (i.e.
> error) instead of truncating the value? Maybe the code does this
> somewhere else? Here seemed like the obvious place for an overflow
> check as we go from sector_t to int.
>
The behavior should still be the same. It will get truncated, unfortunately. I
don't think we can actually change this behavior and return zero instead of
truncating it.
> --D
>
> > +
> > + error = put_user(ur_block, p);
> > +
> > + return error;
> > }
> >
> > /**
> > --
> > 2.20.1
> >
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info
2019-07-31 23:28 ` Darrick J. Wong
@ 2019-08-02 9:51 ` Carlos Maiolino
2019-08-02 15:15 ` Darrick J. Wong
2019-08-06 5:39 ` Christoph Hellwig
0 siblings, 2 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-02 9:51 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
> >
> > STATIC int
> > xfs_vn_fiemap(
> > - struct inode *inode,
> > - struct fiemap_extent_info *fieinfo,
> > - u64 start,
> > - u64 length)
> > + struct inode *inode,
> > + struct fiemap_extent_info *fieinfo)
> > {
> > - int error;
> > + u64 start = fieinfo->fi_start;
> > + u64 length = fieinfo->fi_len;
> > + int error;
>
> Would be nice if the variable name indentation was consistent here, but
> otherwise the xfs part looks ok.
These fields are removed on the next patch, updating it is really required?
>
> >
> > xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
> > if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index d5e7c744aea6..7b744b7de24e 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1705,11 +1705,14 @@ extern bool may_open_dev(const struct path *path);
> > * VFS FS_IOC_FIEMAP helper definitions.
> > */
> > struct fiemap_extent_info {
> > - unsigned int fi_flags; /* Flags as passed from user */
> > - unsigned int fi_extents_mapped; /* Number of mapped extents */
> > - unsigned int fi_extents_max; /* Size of fiemap_extent array */
> > - struct fiemap_extent __user *fi_extents_start; /* Start of
> > - fiemap_extent array */
> > + unsigned int fi_flags; /* Flags as passed from user */
> > + u64 fi_start;
> > + u64 fi_len;
>
> Comments for these two new fields?
Sure, how about this:
u64 fi_start; /* Logical offset at which
start mapping */
u64 fi_len; /* Logical length of mapping
the caller cares about */
btw, Above indentation won't match final result
Christoph, may I keep your reviewed tag by updating the comments as above?
Otherwise I'll just remove your tag
>
> --D
>
> > + unsigned int fi_extents_mapped; /* Number of mapped extents */
> > + unsigned int fi_extents_max; /* Size of fiemap_extent array */
> > + struct fiemap_extent __user *fi_extents_start; /* Start of
> > + fiemap_extent
> > + array */
> > };
> > int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> > u64 phys, u64 len, u32 flags);
> > @@ -1841,8 +1844,7 @@ struct inode_operations {
> > int (*setattr) (struct dentry *, struct iattr *);
> > int (*getattr) (const struct path *, struct kstat *, u32, unsigned int);
> > ssize_t (*listxattr) (struct dentry *, char *, size_t);
> > - int (*fiemap)(struct inode *, struct fiemap_extent_info *, u64 start,
> > - u64 len);
> > + int (*fiemap)(struct inode *, struct fiemap_extent_info *);
> > int (*update_time)(struct inode *, struct timespec64 *, int);
> > int (*atomic_open)(struct inode *, struct dentry *,
> > struct file *, unsigned open_flag,
> > @@ -3199,11 +3201,10 @@ extern int vfs_readlink(struct dentry *, char __user *, int);
> >
> > extern int __generic_block_fiemap(struct inode *inode,
> > struct fiemap_extent_info *fieinfo,
> > - loff_t start, loff_t len,
> > get_block_t *get_block);
> > extern int generic_block_fiemap(struct inode *inode,
> > - struct fiemap_extent_info *fieinfo, u64 start,
> > - u64 len, get_block_t *get_block);
> > + struct fiemap_extent_info *fieinfo,
> > + get_block_t *get_block);
> >
> > extern struct file_system_type *get_filesystem(struct file_system_type *fs);
> > extern void put_filesystem(struct file_system_type *fs);
> > --
> > 2.20.1
> >
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
` (8 preceding siblings ...)
2019-07-31 14:12 ` [PATCH 9/9] xfs: Get rid of ->bmap Carlos Maiolino
@ 2019-08-02 10:20 ` Carlos Maiolino
9 siblings, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-02 10:20 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
On Wed, Jul 31, 2019 at 04:12:36PM +0200, Carlos Maiolino wrote:
> Hi.
Please, disconsider this patchset, I wrongly rebased it and there are many
missing changes that should be done to properly rebase it agains 5.3
>
> This is the 4th version of the complete series with the goal to deprecate and
> eventually remove ->bmap() interface, in lieu of FIEMAP.
>
> Besides the rebase of the patchset against latest Linus's tree, the only changes
> in this patchset regarding to the previous version, are concentrated in patches
> 4/9 and 8/9.
>
> In patch 4 (fibmap: Use bmap instead of ->bmap method in ioctl_fibmap), the
> difference compared with previous version, is a change in ioclt_fibmap() return
> value, I spotted while testing this new version with filesystems using data
> inlined in inodes. It now returns 0 in case of error instead an error value,
> otherwise it would be an user interface change.
>
>
> In patch 8 (Use FIEMAP for FIBMAP calls), there are minor changes regarding V3.
> It just contains a coding style fix pointed by Andreas in the previous version,
> but now, it also include changes to all filesystems which supports both FIEMAP
> and FIBMAP, and need some sort of special handling (like inlined data, reflinked
> inodes, etc).
>
> Again, Patch 9 is xfs-specific removal of ->bmap() interface, without any
> changes compared to the previous version.
>
>
>
> I do apologize for taking so long to rework this patchset, I've got busy with
> other stuff.
>
> Comments are appreciated, specially regarding if the error values returned by
> ioctl_fibmap() make sense.
>
>
> Cheers
>
> Carlos Maiolino (9):
> fs: Enable bmap() function to properly return errors
> cachefiles: drop direct usage of ->bmap method.
> ecryptfs: drop direct calls to ->bmap
> fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
> fs: Move start and length fiemap fields into fiemap_extent_info
> iomap: Remove length and start fields from iomap_fiemap
> fiemap: Use a callback to fill fiemap extents
> Use FIEMAP for FIBMAP calls
> xfs: Get rid of ->bmap
>
> drivers/md/md-bitmap.c | 16 ++++--
> fs/bad_inode.c | 3 +-
> fs/btrfs/inode.c | 5 +-
> fs/cachefiles/rdwr.c | 27 ++++-----
> fs/ecryptfs/mmap.c | 16 ++----
> fs/ext2/ext2.h | 3 +-
> fs/ext2/inode.c | 6 +-
> fs/ext4/ext4.h | 3 +-
> fs/ext4/extents.c | 15 +++--
> fs/f2fs/data.c | 15 ++++-
> fs/f2fs/f2fs.h | 3 +-
> fs/gfs2/inode.c | 9 ++-
> fs/hpfs/file.c | 4 +-
> fs/inode.c | 105 +++++++++++++++++++++++++++++----
> fs/ioctl.c | 128 ++++++++++++++++++++++++++---------------
> fs/iomap.c | 40 ++-----------
> fs/jbd2/journal.c | 22 ++++---
> fs/nilfs2/inode.c | 5 +-
> fs/nilfs2/nilfs.h | 3 +-
> fs/ocfs2/extent_map.c | 13 ++++-
> fs/ocfs2/extent_map.h | 3 +-
> fs/overlayfs/inode.c | 5 +-
> fs/xfs/xfs_aops.c | 24 --------
> fs/xfs/xfs_iops.c | 19 +++---
> fs/xfs/xfs_trace.h | 1 -
> include/linux/fs.h | 33 +++++++----
> include/linux/iomap.h | 2 +-
> mm/page_io.c | 11 ++--
> 28 files changed, 320 insertions(+), 219 deletions(-)
>
> --
> 2.20.1
>
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] Use FIEMAP for FIBMAP calls
2019-07-31 23:22 ` Darrick J. Wong
2019-07-31 23:31 ` Darrick J. Wong
@ 2019-08-02 13:48 ` Carlos Maiolino
2019-08-02 15:29 ` Darrick J. Wong
1 sibling, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-02 13:48 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
> > -#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC|FIEMAP_FLAG_XATTR)
> > +#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC | \
> > + FIEMAP_FLAG_XATTR| \
> > + FIEMAP_KERNEL_FIBMAP)
> >
> > static int ext4_xattr_fiemap(struct inode *inode,
> > struct fiemap_extent_info *fieinfo)
> > @@ -5048,6 +5050,9 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > if (ext4_has_inline_data(inode)) {
> > int has_inline = 1;
> >
> > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> > + return -EINVAL;
>
> Wouldn't the inline data case be caught by fiemap_bmap and turned into
> -EINVAL?
Yes, it does, but until ext4_fiemap() returns the extent with the INLINE flag,
it does need to go through the whole fiemap mapping mechanism when we already
know the result... So, instead of letting the ext4_fiemap() map the extent, just
take the shortcut and return -EINVAL directly.
The check in fiemap_bmap() is a 'safe measure' (if it does have other name I
don't know :), but if the filesystem already knows it's gonna fall into an
inline inode, taking the shortcut is better, isn't it?
> > + return 1;
> > + return 0;
> > +}
> > +
> > +static int bmap_fiemap(struct inode *inode, sector_t *block)
> > +{
> > + struct fiemap_extent_info fieinfo = { 0, };
> > + struct fiemap_extent fextent;
> > + u64 start = *block << inode->i_blkbits;
> > + int error = -EINVAL;
> > +
> > + fextent.fe_logical = 0;
> > + fextent.fe_physical = 0;
> > + fieinfo.fi_extents_max = 1;
> > + fieinfo.fi_extents_mapped = 0;
> > + fieinfo.fi_cb_data = &fextent;
> > + fieinfo.fi_start = start;
> > + fieinfo.fi_len = 1 << inode->i_blkbits;
> > + fieinfo.fi_cb = fiemap_fill_kernel_extent;
> > + fieinfo.fi_flags = (FIEMAP_KERNEL_FIBMAP | FIEMAP_FLAG_SYNC);
> > +
> > + error = inode->i_op->fiemap(inode, &fieinfo);
> > +
> > + if (error)
> > + return error;
> > +
> > + if (fieinfo.fi_flags & (FIEMAP_EXTENT_UNKNOWN |
> > + FIEMAP_EXTENT_ENCODED |
> > + FIEMAP_EXTENT_DATA_INLINE |
> > + FIEMAP_EXTENT_UNWRITTEN |
> > + FIEMAP_EXTENT_SHARED))
> > + return -EINVAL;
> > +
> > + *block = (fextent.fe_physical +
> > + (start - fextent.fe_logical)) >> inode->i_blkbits;
> > +
> > + return error;
> > +}
> > +
> > /**
> > * bmap - find a block number in a file
> > * @inode: inode owning the block number being requested
> > @@ -1591,10 +1663,15 @@ EXPORT_SYMBOL(iput);
> > */
> > int bmap(struct inode *inode, sector_t *block)
> > {
> > - if (!inode->i_mapping->a_ops->bmap)
> > + if (inode->i_op->fiemap)
> > + return bmap_fiemap(inode, block);
> > +
> > + if (inode->i_mapping->a_ops->bmap)
> > + *block = inode->i_mapping->a_ops->bmap(inode->i_mapping,
> > + *block);
> > + else
> > return -EINVAL;
> >
> > - *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
> > return 0;
> > }
> > EXPORT_SYMBOL(bmap);
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index d72696c222de..0759ac6e4c7e 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -77,11 +77,8 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> > return error;
> > }
> >
> > -#define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC)
> > -#define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED)
> > -#define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE)
> > -int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > - u64 phys, u64 len, u32 flags)
> > +static int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo,
> > + u64 logical, u64 phys, u64 len, u32 flags)
> > {
> > struct fiemap_extent extent;
> > struct fiemap_extent __user *dest = fieinfo->fi_cb_data;
> > @@ -89,17 +86,17 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > /* only count the extents */
> > if (fieinfo->fi_extents_max == 0) {
> > fieinfo->fi_extents_mapped++;
> > - return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> > + goto out;
> > }
> >
> > if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
> > return 1;
> >
> > - if (flags & SET_UNKNOWN_FLAGS)
> > + if (flags & FIEMAP_EXTENT_DELALLOC)
> > flags |= FIEMAP_EXTENT_UNKNOWN;
> > - if (flags & SET_NO_UNMOUNTED_IO_FLAGS)
> > + if (flags & FIEMAP_EXTENT_DATA_ENCRYPTED)
> > flags |= FIEMAP_EXTENT_ENCODED;
> > - if (flags & SET_NOT_ALIGNED_FLAGS)
>
> It's too bad that we lose the "not aligned" semantic meaning here.
May you explain a bit better what you mean? We don't lose it, just the define
goes away, the reason I dropped these defines is because the same flags are used
in both functions, fiemap_fill_{user,kernel}_extent(), and I didn't think
defining them on both places (or in fs.h) has any benefit here, so I opted to
remove them.
>
> > + if (flags & (FIEMAP_EXTENT_DATA_TAIL | FIEMAP_EXTENT_DATA_INLINE))
> > flags |= FIEMAP_EXTENT_NOT_ALIGNED;
>
> Why doesn't this function just call fiemap_fill_kernel_extent to fill
> out the onstack @extent structure? We've now implemented "fill out out
> a struct fiemap_extent" twice.
fiemap_fill_{user, kernel}_extent() have different purposes, and the big
difference is one handles a userspace pointer memory and the other don't. IIRC
the original proposal was some sort of sharing a single function, but then
Christoph suggested a new design, using different functions as callbacks.
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index b485190b7ecd..18a798e9076b 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -1113,6 +1113,11 @@ xfs_vn_fiemap(
> > struct fiemap_extent_info *fieinfo)
> > {
> > int error;
> > + struct xfs_inode *ip = XFS_I(inode);
>
> Would you mind fixing the indentation to match usual xfs style?
Sure, will fix it
>
> > +
> > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> > + if (xfs_is_reflink_inode(ip) || XFS_IS_REALTIME_INODE(ip))
> > + return -EINVAL;
>
> The xfs part looks ok to me.
>
> --D
>
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] Use FIEMAP for FIBMAP calls
2019-07-31 23:31 ` Darrick J. Wong
@ 2019-08-02 13:52 ` Carlos Maiolino
2019-08-06 5:41 ` Christoph Hellwig
1 sibling, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-02 13:52 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index b485190b7ecd..18a798e9076b 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -1113,6 +1113,11 @@ xfs_vn_fiemap(
> > > struct fiemap_extent_info *fieinfo)
> > > {
> > > int error;
> > > + struct xfs_inode *ip = XFS_I(inode);
> >
> > Would you mind fixing the indentation to match usual xfs style?
> >
> > > +
> > > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> > > + if (xfs_is_reflink_inode(ip) || XFS_IS_REALTIME_INODE(ip))
> > > + return -EINVAL;
> >
> > The xfs part looks ok to me.
>
> No it doesn't. This check ought to be xfs_is_cow_inode() to match the
> code being removed in the last patch:
>
> if ((fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) &&
> (xfs_is_cow_inode(ip) || XFS_IS_REALTIME_INODE(ip)))
> return -EINVAL;
>
Agreed, will fix
> > --D
> >
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-02 9:19 ` Carlos Maiolino
@ 2019-08-02 15:14 ` Darrick J. Wong
2019-08-05 10:27 ` Carlos Maiolino
0 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2019-08-02 15:14 UTC (permalink / raw)
To: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Fri, Aug 02, 2019 at 11:19:39AM +0200, Carlos Maiolino wrote:
> Hi Darrick.
>
> > > + return error;
> > > +
> > > + block = ur_block;
> > > + error = bmap(inode, &block);
> > > +
> > > + if (error)
> > > + ur_block = 0;
> > > + else
> > > + ur_block = block;
> >
> > What happens if ur_block > INT_MAX? Shouldn't we return zero (i.e.
> > error) instead of truncating the value? Maybe the code does this
> > somewhere else? Here seemed like the obvious place for an overflow
> > check as we go from sector_t to int.
> >
>
> The behavior should still be the same. It will get truncated, unfortunately. I
> don't think we can actually change this behavior and return zero instead of
> truncating it.
But that's even worse, because the programs that rely on FIBMAP will now
receive *incorrect* results that may point at a different file and
definitely do not point at the correct file block.
Note also that the iomap (and therefore xfs) implementation WARNs on
integer overflow and returns 0 (error) to prevent an incorrect access.
--D
> > --D
> >
> > > +
> > > + error = put_user(ur_block, p);
> > > +
> > > + return error;
> > > }
> > >
> > > /**
> > > --
> > > 2.20.1
> > >
>
> --
> Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info
2019-08-02 9:51 ` Carlos Maiolino
@ 2019-08-02 15:15 ` Darrick J. Wong
2019-08-05 9:40 ` Carlos Maiolino
2019-08-06 5:39 ` Christoph Hellwig
1 sibling, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2019-08-02 15:15 UTC (permalink / raw)
To: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Fri, Aug 02, 2019 at 11:51:16AM +0200, Carlos Maiolino wrote:
> > >
> > > STATIC int
> > > xfs_vn_fiemap(
> > > - struct inode *inode,
> > > - struct fiemap_extent_info *fieinfo,
> > > - u64 start,
> > > - u64 length)
> > > + struct inode *inode,
> > > + struct fiemap_extent_info *fieinfo)
> > > {
> > > - int error;
> > > + u64 start = fieinfo->fi_start;
> > > + u64 length = fieinfo->fi_len;
> > > + int error;
> >
> > Would be nice if the variable name indentation was consistent here, but
> > otherwise the xfs part looks ok.
>
> These fields are removed on the next patch, updating it is really required?
Yes, please.
> >
> > >
> > > xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
> > > if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index d5e7c744aea6..7b744b7de24e 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -1705,11 +1705,14 @@ extern bool may_open_dev(const struct path *path);
> > > * VFS FS_IOC_FIEMAP helper definitions.
> > > */
> > > struct fiemap_extent_info {
> > > - unsigned int fi_flags; /* Flags as passed from user */
> > > - unsigned int fi_extents_mapped; /* Number of mapped extents */
> > > - unsigned int fi_extents_max; /* Size of fiemap_extent array */
> > > - struct fiemap_extent __user *fi_extents_start; /* Start of
> > > - fiemap_extent array */
> > > + unsigned int fi_flags; /* Flags as passed from user */
> > > + u64 fi_start;
> > > + u64 fi_len;
> >
> > Comments for these two new fields?
>
> Sure, how about this:
>
> u64 fi_start; /* Logical offset at which
> start mapping */
> u64 fi_len; /* Logical length of mapping
> the caller cares about */
>
>
> btw, Above indentation won't match final result
Looks good to me.
--D
>
> Christoph, may I keep your reviewed tag by updating the comments as above?
> Otherwise I'll just remove your tag
>
> >
> > --D
> >
> > > + unsigned int fi_extents_mapped; /* Number of mapped extents */
> > > + unsigned int fi_extents_max; /* Size of fiemap_extent array */
> > > + struct fiemap_extent __user *fi_extents_start; /* Start of
> > > + fiemap_extent
> > > + array */
> > > };
> > > int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> > > u64 phys, u64 len, u32 flags);
> > > @@ -1841,8 +1844,7 @@ struct inode_operations {
> > > int (*setattr) (struct dentry *, struct iattr *);
> > > int (*getattr) (const struct path *, struct kstat *, u32, unsigned int);
> > > ssize_t (*listxattr) (struct dentry *, char *, size_t);
> > > - int (*fiemap)(struct inode *, struct fiemap_extent_info *, u64 start,
> > > - u64 len);
> > > + int (*fiemap)(struct inode *, struct fiemap_extent_info *);
> > > int (*update_time)(struct inode *, struct timespec64 *, int);
> > > int (*atomic_open)(struct inode *, struct dentry *,
> > > struct file *, unsigned open_flag,
> > > @@ -3199,11 +3201,10 @@ extern int vfs_readlink(struct dentry *, char __user *, int);
> > >
> > > extern int __generic_block_fiemap(struct inode *inode,
> > > struct fiemap_extent_info *fieinfo,
> > > - loff_t start, loff_t len,
> > > get_block_t *get_block);
> > > extern int generic_block_fiemap(struct inode *inode,
> > > - struct fiemap_extent_info *fieinfo, u64 start,
> > > - u64 len, get_block_t *get_block);
> > > + struct fiemap_extent_info *fieinfo,
> > > + get_block_t *get_block);
> > >
> > > extern struct file_system_type *get_filesystem(struct file_system_type *fs);
> > > extern void put_filesystem(struct file_system_type *fs);
> > > --
> > > 2.20.1
> > >
>
> --
> Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] Use FIEMAP for FIBMAP calls
2019-08-02 13:48 ` Carlos Maiolino
@ 2019-08-02 15:29 ` Darrick J. Wong
2019-08-05 10:38 ` Carlos Maiolino
2019-08-06 5:46 ` Christoph Hellwig
0 siblings, 2 replies; 47+ messages in thread
From: Darrick J. Wong @ 2019-08-02 15:29 UTC (permalink / raw)
To: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Fri, Aug 02, 2019 at 03:48:17PM +0200, Carlos Maiolino wrote:
> > > -#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC|FIEMAP_FLAG_XATTR)
> > > +#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC | \
> > > + FIEMAP_FLAG_XATTR| \
> > > + FIEMAP_KERNEL_FIBMAP)
> > >
> > > static int ext4_xattr_fiemap(struct inode *inode,
> > > struct fiemap_extent_info *fieinfo)
> > > @@ -5048,6 +5050,9 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > > if (ext4_has_inline_data(inode)) {
> > > int has_inline = 1;
> > >
> > > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> > > + return -EINVAL;
> >
> > Wouldn't the inline data case be caught by fiemap_bmap and turned into
> > -EINVAL?
>
> Yes, it does, but until ext4_fiemap() returns the extent with the INLINE flag,
> it does need to go through the whole fiemap mapping mechanism when we already
> know the result... So, instead of letting the ext4_fiemap() map the extent, just
> take the shortcut and return -EINVAL directly.
>
> The check in fiemap_bmap() is a 'safe measure' (if it does have other name I
> don't know :), but if the filesystem already knows it's gonna fall into an
> inline inode, taking the shortcut is better, isn't it?
I suppose so. Just wondering, that was all... :)
>
> > > + return 1;
> > > + return 0;
> > > +}
> > > +
> > > +static int bmap_fiemap(struct inode *inode, sector_t *block)
> > > +{
> > > + struct fiemap_extent_info fieinfo = { 0, };
> > > + struct fiemap_extent fextent;
> > > + u64 start = *block << inode->i_blkbits;
> > > + int error = -EINVAL;
> > > +
> > > + fextent.fe_logical = 0;
> > > + fextent.fe_physical = 0;
> > > + fieinfo.fi_extents_max = 1;
> > > + fieinfo.fi_extents_mapped = 0;
> > > + fieinfo.fi_cb_data = &fextent;
> > > + fieinfo.fi_start = start;
> > > + fieinfo.fi_len = 1 << inode->i_blkbits;
> > > + fieinfo.fi_cb = fiemap_fill_kernel_extent;
> > > + fieinfo.fi_flags = (FIEMAP_KERNEL_FIBMAP | FIEMAP_FLAG_SYNC);
> > > +
> > > + error = inode->i_op->fiemap(inode, &fieinfo);
> > > +
> > > + if (error)
> > > + return error;
> > > +
> > > + if (fieinfo.fi_flags & (FIEMAP_EXTENT_UNKNOWN |
> > > + FIEMAP_EXTENT_ENCODED |
> > > + FIEMAP_EXTENT_DATA_INLINE |
> > > + FIEMAP_EXTENT_UNWRITTEN |
> > > + FIEMAP_EXTENT_SHARED))
> > > + return -EINVAL;
> > > +
> > > + *block = (fextent.fe_physical +
> > > + (start - fextent.fe_logical)) >> inode->i_blkbits;
> > > +
> > > + return error;
> > > +}
> > > +
> > > /**
> > > * bmap - find a block number in a file
> > > * @inode: inode owning the block number being requested
> > > @@ -1591,10 +1663,15 @@ EXPORT_SYMBOL(iput);
> > > */
> > > int bmap(struct inode *inode, sector_t *block)
> > > {
> > > - if (!inode->i_mapping->a_ops->bmap)
> > > + if (inode->i_op->fiemap)
> > > + return bmap_fiemap(inode, block);
> > > +
> > > + if (inode->i_mapping->a_ops->bmap)
> > > + *block = inode->i_mapping->a_ops->bmap(inode->i_mapping,
> > > + *block);
> > > + else
> > > return -EINVAL;
> > >
> > > - *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
> > > return 0;
> > > }
> > > EXPORT_SYMBOL(bmap);
> > > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > > index d72696c222de..0759ac6e4c7e 100644
> > > --- a/fs/ioctl.c
> > > +++ b/fs/ioctl.c
> > > @@ -77,11 +77,8 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> > > return error;
> > > }
> > >
> > > -#define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC)
> > > -#define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED)
> > > -#define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE)
> > > -int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > > - u64 phys, u64 len, u32 flags)
> > > +static int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo,
> > > + u64 logical, u64 phys, u64 len, u32 flags)
> > > {
> > > struct fiemap_extent extent;
> > > struct fiemap_extent __user *dest = fieinfo->fi_cb_data;
> > > @@ -89,17 +86,17 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > > /* only count the extents */
> > > if (fieinfo->fi_extents_max == 0) {
> > > fieinfo->fi_extents_mapped++;
> > > - return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> > > + goto out;
> > > }
> > >
> > > if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
> > > return 1;
> > >
> > > - if (flags & SET_UNKNOWN_FLAGS)
> > > + if (flags & FIEMAP_EXTENT_DELALLOC)
> > > flags |= FIEMAP_EXTENT_UNKNOWN;
> > > - if (flags & SET_NO_UNMOUNTED_IO_FLAGS)
> > > + if (flags & FIEMAP_EXTENT_DATA_ENCRYPTED)
> > > flags |= FIEMAP_EXTENT_ENCODED;
> > > - if (flags & SET_NOT_ALIGNED_FLAGS)
> >
> > It's too bad that we lose the "not aligned" semantic meaning here.
>
> May you explain a bit better what you mean? We don't lose it, just the define
> goes away, the reason I dropped these defines is because the same flags are used
> in both functions, fiemap_fill_{user,kernel}_extent(), and I didn't think
> defining them on both places (or in fs.h) has any benefit here, so I opted to
> remove them.
Eh, I changed my mind. It's easy enough to tell which flags map to "No
umounted IO" from the code even if the #defines go away.
> >
> > > + if (flags & (FIEMAP_EXTENT_DATA_TAIL | FIEMAP_EXTENT_DATA_INLINE))
> > > flags |= FIEMAP_EXTENT_NOT_ALIGNED;
> >
> > Why doesn't this function just call fiemap_fill_kernel_extent to fill
> > out the onstack @extent structure? We've now implemented "fill out out
> > a struct fiemap_extent" twice.
>
> fiemap_fill_{user, kernel}_extent() have different purposes, and the big
> difference is one handles a userspace pointer memory and the other don't. IIRC
> the original proposal was some sort of sharing a single function, but then
> Christoph suggested a new design, using different functions as callbacks.
It's harder for me to tell when I don't have a branch containing the
final product to look at, but I'd have thought that _fill_kernel fills
out an in-kernel fiemap extent; and then _fill_user would declare one on
the stack, call _fill_kernel to set the fields, and then copy_to_user?
(But maybe the code already does this and I can't tell...)
>
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index b485190b7ecd..18a798e9076b 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -1113,6 +1113,11 @@ xfs_vn_fiemap(
> > > struct fiemap_extent_info *fieinfo)
> > > {
> > > int error;
> > > + struct xfs_inode *ip = XFS_I(inode);
> >
> > Would you mind fixing the indentation to match usual xfs style?
>
> Sure, will fix it
>
>
> >
> > > +
> > > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> > > + if (xfs_is_reflink_inode(ip) || XFS_IS_REALTIME_INODE(ip))
> > > + return -EINVAL;
> >
> > The xfs part looks ok to me.
> >
> > --D
> >
>
> --
> Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info
2019-08-02 15:15 ` Darrick J. Wong
@ 2019-08-05 9:40 ` Carlos Maiolino
0 siblings, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-05 9:40 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Fri, Aug 02, 2019 at 08:15:19AM -0700, Darrick J. Wong wrote:
> On Fri, Aug 02, 2019 at 11:51:16AM +0200, Carlos Maiolino wrote:
> > > >
> > > > STATIC int
> > > > xfs_vn_fiemap(
> > > > - struct inode *inode,
> > > > - struct fiemap_extent_info *fieinfo,
> > > > - u64 start,
> > > > - u64 length)
> > > > + struct inode *inode,
> > > > + struct fiemap_extent_info *fieinfo)
> > > > {
> > > > - int error;
> > > > + u64 start = fieinfo->fi_start;
> > > > + u64 length = fieinfo->fi_len;
> > > > + int error;
> > >
> > > Would be nice if the variable name indentation was consistent here, but
> > > otherwise the xfs part looks ok.
> >
> > These fields are removed on the next patch, updating it is really required?
>
> Yes, please.
NP then
> > > > + u64 fi_start;
> > > > + u64 fi_len;
> > >
> > > Comments for these two new fields?
> >
> > Sure, how about this:
> >
> > u64 fi_start; /* Logical offset at which
> > start mapping */
> > u64 fi_len; /* Logical length of mapping
> > the caller cares about */
> >
> >
> > btw, Above indentation won't match final result
>
> Looks good to me.
Will update the fields, thanks for the review
>
> >
> > Christoph, may I keep your reviewed tag by updating the comments as above?
> > Otherwise I'll just remove your tag
> >
> > >
> > > --D
> > >
> > > > + unsigned int fi_extents_mapped; /* Number of mapped extents */
> > > > + unsigned int fi_extents_max; /* Size of fiemap_extent array */
> > > > + struct fiemap_extent __user *fi_extents_start; /* Start of
> > > > + fiemap_extent
> > > > + array */
> > > > };
> > > > int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> > > > u64 phys, u64 len, u32 flags);
> > > > @@ -1841,8 +1844,7 @@ struct inode_operations {
> > > > int (*setattr) (struct dentry *, struct iattr *);
> > > > int (*getattr) (const struct path *, struct kstat *, u32, unsigned int);
> > > > ssize_t (*listxattr) (struct dentry *, char *, size_t);
> > > > - int (*fiemap)(struct inode *, struct fiemap_extent_info *, u64 start,
> > > > - u64 len);
> > > > + int (*fiemap)(struct inode *, struct fiemap_extent_info *);
> > > > int (*update_time)(struct inode *, struct timespec64 *, int);
> > > > int (*atomic_open)(struct inode *, struct dentry *,
> > > > struct file *, unsigned open_flag,
> > > > @@ -3199,11 +3201,10 @@ extern int vfs_readlink(struct dentry *, char __user *, int);
> > > >
> > > > extern int __generic_block_fiemap(struct inode *inode,
> > > > struct fiemap_extent_info *fieinfo,
> > > > - loff_t start, loff_t len,
> > > > get_block_t *get_block);
> > > > extern int generic_block_fiemap(struct inode *inode,
> > > > - struct fiemap_extent_info *fieinfo, u64 start,
> > > > - u64 len, get_block_t *get_block);
> > > > + struct fiemap_extent_info *fieinfo,
> > > > + get_block_t *get_block);
> > > >
> > > > extern struct file_system_type *get_filesystem(struct file_system_type *fs);
> > > > extern void put_filesystem(struct file_system_type *fs);
> > > > --
> > > > 2.20.1
> > > >
> >
> > --
> > Carlos
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-02 15:14 ` Darrick J. Wong
@ 2019-08-05 10:27 ` Carlos Maiolino
2019-08-05 15:12 ` Darrick J. Wong
0 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-05 10:27 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Fri, Aug 02, 2019 at 08:14:00AM -0700, Darrick J. Wong wrote:
> On Fri, Aug 02, 2019 at 11:19:39AM +0200, Carlos Maiolino wrote:
> > Hi Darrick.
> >
> > > > + return error;
> > > > +
> > > > + block = ur_block;
> > > > + error = bmap(inode, &block);
> > > > +
> > > > + if (error)
> > > > + ur_block = 0;
> > > > + else
> > > > + ur_block = block;
> > >
> > > What happens if ur_block > INT_MAX? Shouldn't we return zero (i.e.
> > > error) instead of truncating the value? Maybe the code does this
> > > somewhere else? Here seemed like the obvious place for an overflow
> > > check as we go from sector_t to int.
> > >
> >
> > The behavior should still be the same. It will get truncated, unfortunately. I
> > don't think we can actually change this behavior and return zero instead of
> > truncating it.
>
> But that's even worse, because the programs that rely on FIBMAP will now
> receive *incorrect* results that may point at a different file and
> definitely do not point at the correct file block.
How is this worse? This is exactly what happens today, on the original FIBMAP
implementation.
Maybe I am not seeing something or having a different thinking you have, but
this is the behavior we have now, without my patches. And we can't really change
it; the user view of this implementation.
That's why I didn't try to change the result, so the truncation still happens.
>
> Note also that the iomap (and therefore xfs) implementation WARNs on
> integer overflow and returns 0 (error) to prevent an incorrect access.
It does not really prevent anything. It just issue a warning saying the result
will be truncated, in an attempt to notify the FIBMAP interface user that he/she
can't trust the result, but it does not prevent a truncated result to be
returned. And IIRC, iomap is the only interface now that cares about issuing a
warning.
I think the *best* we could do here, is to make the new bmap() to issue the same
kind of WARN() iomap does, but we can't really change the end result.
>
> --D
>
> > > --D
> > >
> > > > +
> > > > + error = put_user(ur_block, p);
> > > > +
> > > > + return error;
> > > > }
> > > >
> > > > /**
> > > > --
> > > > 2.20.1
> > > >
> >
> > --
> > Carlos
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] Use FIEMAP for FIBMAP calls
2019-08-02 15:29 ` Darrick J. Wong
@ 2019-08-05 10:38 ` Carlos Maiolino
2019-08-06 5:46 ` Christoph Hellwig
1 sibling, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-05 10:38 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
> > >
> > > Why doesn't this function just call fiemap_fill_kernel_extent to fill
> > > out the onstack @extent structure? We've now implemented "fill out out
> > > a struct fiemap_extent" twice.
> >
> > fiemap_fill_{user, kernel}_extent() have different purposes, and the big
> > difference is one handles a userspace pointer memory and the other don't. IIRC
> > the original proposal was some sort of sharing a single function, but then
> > Christoph suggested a new design, using different functions as callbacks.
>
> It's harder for me to tell when I don't have a branch containing the
> final product to look at,
Good, I though I was the only one having issues with it :)
You can see the work here:
https://github.com/cmaiolino/linux/commits/FIEMAP_V5
^ This already includes changes addressing your concerns as we discussed int
this thread btw.
> but I'd have thought that _fill_kernel fills
> out an in-kernel fiemap extent; and then _fill_user would declare one on
> the stack, call _fill_kernel to set the fields, and then copy_to_user?
None of those functions will declare a fiemap_extent, the fiemap extent will be
declared before (in ioctl_fiemap() or bmap_fiemap()) calling either function and
then passed to the proper callback via fieinfo.fi_cb_data. This will be a user
or a kernel memory address, and the callbacks will handle the memory accordingly.
Cheers
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-05 10:27 ` Carlos Maiolino
@ 2019-08-05 15:12 ` Darrick J. Wong
2019-08-06 5:38 ` Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 47+ messages in thread
From: Darrick J. Wong @ 2019-08-05 15:12 UTC (permalink / raw)
To: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Mon, Aug 05, 2019 at 12:27:30PM +0200, Carlos Maiolino wrote:
> On Fri, Aug 02, 2019 at 08:14:00AM -0700, Darrick J. Wong wrote:
> > On Fri, Aug 02, 2019 at 11:19:39AM +0200, Carlos Maiolino wrote:
> > > Hi Darrick.
> > >
> > > > > + return error;
> > > > > +
> > > > > + block = ur_block;
> > > > > + error = bmap(inode, &block);
> > > > > +
> > > > > + if (error)
> > > > > + ur_block = 0;
> > > > > + else
> > > > > + ur_block = block;
> > > >
> > > > What happens if ur_block > INT_MAX? Shouldn't we return zero (i.e.
> > > > error) instead of truncating the value? Maybe the code does this
> > > > somewhere else? Here seemed like the obvious place for an overflow
> > > > check as we go from sector_t to int.
> > > >
> > >
> > > The behavior should still be the same. It will get truncated, unfortunately. I
> > > don't think we can actually change this behavior and return zero instead of
> > > truncating it.
> >
> > But that's even worse, because the programs that rely on FIBMAP will now
> > receive *incorrect* results that may point at a different file and
> > definitely do not point at the correct file block.
>
> How is this worse? This is exactly what happens today, on the original FIBMAP
> implementation.
Ok, I wasn't being 110% careful with my words. Delete "will now" from
the sentence above.
> Maybe I am not seeing something or having a different thinking you have, but
> this is the behavior we have now, without my patches. And we can't really change
> it; the user view of this implementation.
> That's why I didn't try to change the result, so the truncation still happens.
I understand that we're not generally supposed to change existing
userspace interfaces, but the fact remains that allowing truncated
responses causes *filesystem corruption*.
We know that the most well known FIBMAP callers are bootloaders, and we
know what they do with the information they get -- they use it to record
the block map of boot files. So if the IPL/grub/whatever installer
queries the boot file and the boot file is at block 12345678901 (a
34-bit number), this interface truncates that to 3755744309 (a 32-bit
number) and that's where the bootloader will think its boot files are.
The installation succeeds, the user reboots and *kaboom* the system no
longer boots because the contents of block 3755744309 is not a bootloader.
Worse yet, grub1 used FIBMAP data to record the location of the grub
environment file and installed itself between the MBR and the start of
partition 1. If the environment file is at offset 1234578901, grub will
write status data to its environment file (which it thinks is at
3755744309) and *KABOOM* we've just destroyed whatever was in that
block.
Far better for the bootloader installation script to hit an error and
force the admin to deal with the situation than for the system to become
unbootable. That's *why* the (newer) iomap bmap implementation does not
return truncated mappings, even though the classic implementation does.
The classic code returning truncated results is a broken behavior.
> > Note also that the iomap (and therefore xfs) implementation WARNs on
> > integer overflow and returns 0 (error) to prevent an incorrect access.
>
> It does not really prevent anything. It just issue a warning saying the result
> will be truncated, in an attempt to notify the FIBMAP interface user that he/she
> can't trust the result, but it does not prevent a truncated result to be
I disagree; the iomap bmap implementation /does/ prevent truncated responses:
: static loff_t
: iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,
: void *data, struct iomap *iomap)
: {
: sector_t *bno = data, addr;
:
: if (iomap->type == IOMAP_MAPPED) {
: addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits;
: if (addr > INT_MAX)
: WARN(1, "would truncate bmap result\n");
Notice how we don't set *bno here?
: else
: *bno = addr;
And only set it in the case that there isn't an integer overflow?
: }
: return 0;
: }
:
: /* legacy ->bmap interface. 0 is the error return (!) */
: sector_t
: iomap_bmap(struct address_space *mapping, sector_t bno,
: const struct iomap_ops *ops)
: {
: struct inode *inode = mapping->host;
: loff_t pos = bno << inode->i_blkbits;
: unsigned blocksize = i_blocksize(inode);
:
: if (filemap_write_and_wait(mapping))
: return 0;
:
: bno = 0;
We initialize bno to zero here...
: iomap_apply(inode, pos, blocksize, 0, ops, &bno, iomap_bmap_actor);
...then pass bno's address to the apply function to pass to
iomap_bmap_actor, so either the _actor function set bno or in the case
of overflow it left it set to zero.
: return bno;
: }
> returned. And IIRC, iomap is the only interface now that cares about issuing a
> warning.
>
> I think the *best* we could do here, is to make the new bmap() to issue the same
> kind of WARN() iomap does, but we can't really change the end result.
I'd rather we break legacy code than corrupt filesystems.
--D
>
> >
> > --D
> >
> > > > --D
> > > >
> > > > > +
> > > > > + error = put_user(ur_block, p);
> > > > > +
> > > > > + return error;
> > > > > }
> > > > >
> > > > > /**
> > > > > --
> > > > > 2.20.1
> > > > >
> > >
> > > --
> > > Carlos
>
> --
> Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-05 15:12 ` Darrick J. Wong
@ 2019-08-06 5:38 ` Christoph Hellwig
2019-08-06 12:07 ` Carlos Maiolino
2019-08-06 12:02 ` Carlos Maiolino
2019-08-06 22:41 ` Luis Chamberlain
2 siblings, 1 reply; 47+ messages in thread
From: Christoph Hellwig @ 2019-08-06 5:38 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Mon, Aug 05, 2019 at 08:12:58AM -0700, Darrick J. Wong wrote:
> > returned. And IIRC, iomap is the only interface now that cares about issuing a
> > warning.
> >
> > I think the *best* we could do here, is to make the new bmap() to issue the same
> > kind of WARN() iomap does, but we can't really change the end result.
>
> I'd rather we break legacy code than corrupt filesystems.
This particular patch should keep existing behavior as is, as the intent
is to not change functionality. Throwing in another patch to have saner
error behavior now that we have a saner in-kernel interface that cleary
documents what it is breaking and why on the other hand sounds like a
very good idea.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info
2019-08-02 9:51 ` Carlos Maiolino
2019-08-02 15:15 ` Darrick J. Wong
@ 2019-08-06 5:39 ` Christoph Hellwig
1 sibling, 0 replies; 47+ messages in thread
From: Christoph Hellwig @ 2019-08-06 5:39 UTC (permalink / raw)
To: Darrick J. Wong, linux-fsdevel, hch, adilger, jaegeuk, miklos,
rpeterso, linux-xfs
On Fri, Aug 02, 2019 at 11:51:16AM +0200, Carlos Maiolino wrote:
> Christoph, may I keep your reviewed tag by updating the comments as above?
> Otherwise I'll just remove your tag
Feel free to keep them for any trivial changes. Indentation changes and
adding comments always qualify as trivial.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] Use FIEMAP for FIBMAP calls
2019-07-31 23:31 ` Darrick J. Wong
2019-08-02 13:52 ` Carlos Maiolino
@ 2019-08-06 5:41 ` Christoph Hellwig
1 sibling, 0 replies; 47+ messages in thread
From: Christoph Hellwig @ 2019-08-06 5:41 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Carlos Maiolino, linux-fsdevel, hch, adilger, jaegeuk, miklos,
rpeterso, linux-xfs
Darrick, and chance to not send out a double full quote? I just can't
not find the actual information in this mail even if I try.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] Use FIEMAP for FIBMAP calls
2019-08-02 15:29 ` Darrick J. Wong
2019-08-05 10:38 ` Carlos Maiolino
@ 2019-08-06 5:46 ` Christoph Hellwig
1 sibling, 0 replies; 47+ messages in thread
From: Christoph Hellwig @ 2019-08-06 5:46 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Fri, Aug 02, 2019 at 08:29:02AM -0700, Darrick J. Wong wrote:
> It's harder for me to tell when I don't have a branch containing the
> final product to look at, but I'd have thought that _fill_kernel fills
> out an in-kernel fiemap extent; and then _fill_user would declare one on
> the stack, call _fill_kernel to set the fields, and then copy_to_user?
That works fine for small, fixed-sized structures. But for large
variable sized structures it is very inefficient, as we need to do a
possibly large kernel allocation and just copy it on. Thus I told Carlos
to follow the readdir model with the ->actor (used to be ->filldir)
callback that can fill out the actual kernel or user data directly.
Another example of this high-level model is our struct iov_iter used
in the I/O path.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-05 15:12 ` Darrick J. Wong
2019-08-06 5:38 ` Christoph Hellwig
@ 2019-08-06 12:02 ` Carlos Maiolino
2019-08-06 22:41 ` Luis Chamberlain
2 siblings, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-06 12:02 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
I'll keep my reply to your email short here, because hch's follow-up email
summarizes what I wanted to say, so I'll reply to his email directly.
> : return bno;
> : }
>
> > returned. And IIRC, iomap is the only interface now that cares about issuing a
> > warning.
> >
> > I think the *best* we could do here, is to make the new bmap() to issue the same
> > kind of WARN() iomap does, but we can't really change the end result.
>
> I'd rather we break legacy code than corrupt filesystems.
By now, I wish to apologize Darrick, it was my fault to not pay enough attention
to iomap's bmap implementation as you mentioned. Will keep the discussion topic
on next e-mail.
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-06 5:38 ` Christoph Hellwig
@ 2019-08-06 12:07 ` Carlos Maiolino
2019-08-06 14:48 ` Darrick J. Wong
0 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-06 12:07 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Darrick J. Wong, linux-fsdevel, adilger, jaegeuk, miklos,
rpeterso, linux-xfs
On Tue, Aug 06, 2019 at 07:38:40AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 05, 2019 at 08:12:58AM -0700, Darrick J. Wong wrote:
> > > returned. And IIRC, iomap is the only interface now that cares about issuing a
> > > warning.
> > >
> > > I think the *best* we could do here, is to make the new bmap() to issue the same
> > > kind of WARN() iomap does, but we can't really change the end result.
> >
> > I'd rather we break legacy code than corrupt filesystems.
>
Yes, I have the same feeling, but this patchset does not have the goal to fix
the broken api.
> This particular patch should keep existing behavior as is, as the intent
> is to not change functionality. Throwing in another patch to have saner
> error behavior now that we have a saner in-kernel interface that cleary
> documents what it is breaking and why on the other hand sounds like a
> very good idea.
I totally agree here, and to be honest, I think such change should be in a
different patchset rather than a new patch in this series. I can do it for sure,
but this discussion IMHO should be done not only here in linux-fsdevel, but also
in linux-api, which well, I don't think cc'ing this whole patchset there will do
any good other than keep the change discussion more complicated than it should
be. I'd rather finish the design and implementation of this patchset, and I'll
follow-up it, once it's all set, with a new patch to change the truncation
behavior, it will make the discussion way easier than mixing up subjects. What
you guys think?
Cheers
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-06 12:07 ` Carlos Maiolino
@ 2019-08-06 14:48 ` Darrick J. Wong
2019-08-08 7:17 ` Carlos Maiolino
0 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2019-08-06 14:48 UTC (permalink / raw)
To: Christoph Hellwig, linux-fsdevel, adilger, jaegeuk, miklos,
rpeterso, linux-xfs
On Tue, Aug 06, 2019 at 02:07:24PM +0200, Carlos Maiolino wrote:
> On Tue, Aug 06, 2019 at 07:38:40AM +0200, Christoph Hellwig wrote:
> > On Mon, Aug 05, 2019 at 08:12:58AM -0700, Darrick J. Wong wrote:
> > > > returned. And IIRC, iomap is the only interface now that cares about issuing a
> > > > warning.
> > > >
> > > > I think the *best* we could do here, is to make the new bmap() to issue the same
> > > > kind of WARN() iomap does, but we can't really change the end result.
> > >
> > > I'd rather we break legacy code than corrupt filesystems.
> >
>
> Yes, I have the same feeling, but this patchset does not have the goal to fix
> the broken api.
>
> > This particular patch should keep existing behavior as is, as the intent
> > is to not change functionality. Throwing in another patch to have saner
> > error behavior now that we have a saner in-kernel interface that cleary
> > documents what it is breaking and why on the other hand sounds like a
> > very good idea.
>
> I totally agree here, and to be honest, I think such change should be in a
> different patchset rather than a new patch in this series. I can do it for sure,
> but this discussion IMHO should be done not only here in linux-fsdevel, but also
> in linux-api, which well, I don't think cc'ing this whole patchset there will do
> any good other than keep the change discussion more complicated than it should
> be. I'd rather finish the design and implementation of this patchset, and I'll
> follow-up it, once it's all set, with a new patch to change the truncation
> behavior, it will make the discussion way easier than mixing up subjects. What
> you guys think?
I probably would've fixed the truncation behavior in the old code and
based the fiemap-fibmap conversion on that so that anyone who wants to
backport the behavior change to an old kernel has an easier time of it.
But afterwards probably works just as well since I don't feel like tying
ourselves in more knots over an old interface. ;)
--D
> Cheers
>
>
> --
> Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-05 15:12 ` Darrick J. Wong
2019-08-06 5:38 ` Christoph Hellwig
2019-08-06 12:02 ` Carlos Maiolino
@ 2019-08-06 22:41 ` Luis Chamberlain
2019-08-07 14:42 ` Darrick J. Wong
2019-08-08 7:12 ` Carlos Maiolino
2 siblings, 2 replies; 47+ messages in thread
From: Luis Chamberlain @ 2019-08-06 22:41 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Mon, Aug 05, 2019 at 08:12:58AM -0700, Darrick J. Wong wrote:
> On Mon, Aug 05, 2019 at 12:27:30PM +0200, Carlos Maiolino wrote:
> > On Fri, Aug 02, 2019 at 08:14:00AM -0700, Darrick J. Wong wrote:
> > > On Fri, Aug 02, 2019 at 11:19:39AM +0200, Carlos Maiolino wrote:
> > > > Hi Darrick.
> > > >
> > > > > > + return error;
> > > > > > +
> > > > > > + block = ur_block;
> > > > > > + error = bmap(inode, &block);
> > > > > > +
> > > > > > + if (error)
> > > > > > + ur_block = 0;
> > > > > > + else
> > > > > > + ur_block = block;
> > > > >
> > > > > What happens if ur_block > INT_MAX? Shouldn't we return zero (i.e.
> > > > > error) instead of truncating the value? Maybe the code does this
> > > > > somewhere else? Here seemed like the obvious place for an overflow
> > > > > check as we go from sector_t to int.
> > > > >
> > > >
> > > > The behavior should still be the same. It will get truncated, unfortunately. I
> > > > don't think we can actually change this behavior and return zero instead of
> > > > truncating it.
> > >
> > > But that's even worse, because the programs that rely on FIBMAP will now
> > > receive *incorrect* results that may point at a different file and
> > > definitely do not point at the correct file block.
> >
> > How is this worse? This is exactly what happens today, on the original FIBMAP
> > implementation.
>
> Ok, I wasn't being 110% careful with my words. Delete "will now" from
> the sentence above.
>
> > Maybe I am not seeing something or having a different thinking you have, but
> > this is the behavior we have now, without my patches. And we can't really change
> > it; the user view of this implementation.
> > That's why I didn't try to change the result, so the truncation still happens.
>
> I understand that we're not generally supposed to change existing
> userspace interfaces, but the fact remains that allowing truncated
> responses causes *filesystem corruption*.
>
> We know that the most well known FIBMAP callers are bootloaders, and we
> know what they do with the information they get -- they use it to record
> the block map of boot files. So if the IPL/grub/whatever installer
> queries the boot file and the boot file is at block 12345678901 (a
> 34-bit number), this interface truncates that to 3755744309 (a 32-bit
> number) and that's where the bootloader will think its boot files are.
> The installation succeeds, the user reboots and *kaboom* the system no
> longer boots because the contents of block 3755744309 is not a bootloader.
>
> Worse yet, grub1 used FIBMAP data to record the location of the grub
> environment file and installed itself between the MBR and the start of
> partition 1. If the environment file is at offset 1234578901, grub will
> write status data to its environment file (which it thinks is at
> 3755744309) and *KABOOM* we've just destroyed whatever was in that
> block.
>
> Far better for the bootloader installation script to hit an error and
> force the admin to deal with the situation than for the system to become
> unbootable. That's *why* the (newer) iomap bmap implementation does not
> return truncated mappings, even though the classic implementation does.
>
> The classic code returning truncated results is a broken behavior.
How long as it been broken for? And if we do fix it, I'd just like for
a nice commit lot describing potential risks of not applying it. *If*
the issue exists as-is today, the above contains a lot of information
for addressing potential issues, even if theoretical.
Luis
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-06 22:41 ` Luis Chamberlain
@ 2019-08-07 14:42 ` Darrick J. Wong
2019-08-08 7:12 ` Carlos Maiolino
1 sibling, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2019-08-07 14:42 UTC (permalink / raw)
To: Luis Chamberlain
Cc: linux-fsdevel, hch, adilger, jaegeuk, miklos, rpeterso, linux-xfs
On Tue, Aug 06, 2019 at 10:41:38PM +0000, Luis Chamberlain wrote:
> On Mon, Aug 05, 2019 at 08:12:58AM -0700, Darrick J. Wong wrote:
> > On Mon, Aug 05, 2019 at 12:27:30PM +0200, Carlos Maiolino wrote:
> > > On Fri, Aug 02, 2019 at 08:14:00AM -0700, Darrick J. Wong wrote:
> > > > On Fri, Aug 02, 2019 at 11:19:39AM +0200, Carlos Maiolino wrote:
> > > > > Hi Darrick.
> > > > >
> > > > > > > + return error;
> > > > > > > +
> > > > > > > + block = ur_block;
> > > > > > > + error = bmap(inode, &block);
> > > > > > > +
> > > > > > > + if (error)
> > > > > > > + ur_block = 0;
> > > > > > > + else
> > > > > > > + ur_block = block;
> > > > > >
> > > > > > What happens if ur_block > INT_MAX? Shouldn't we return zero (i.e.
> > > > > > error) instead of truncating the value? Maybe the code does this
> > > > > > somewhere else? Here seemed like the obvious place for an overflow
> > > > > > check as we go from sector_t to int.
> > > > > >
> > > > >
> > > > > The behavior should still be the same. It will get truncated, unfortunately. I
> > > > > don't think we can actually change this behavior and return zero instead of
> > > > > truncating it.
> > > >
> > > > But that's even worse, because the programs that rely on FIBMAP will now
> > > > receive *incorrect* results that may point at a different file and
> > > > definitely do not point at the correct file block.
> > >
> > > How is this worse? This is exactly what happens today, on the original FIBMAP
> > > implementation.
> >
> > Ok, I wasn't being 110% careful with my words. Delete "will now" from
> > the sentence above.
> >
> > > Maybe I am not seeing something or having a different thinking you have, but
> > > this is the behavior we have now, without my patches. And we can't really change
> > > it; the user view of this implementation.
> > > That's why I didn't try to change the result, so the truncation still happens.
> >
> > I understand that we're not generally supposed to change existing
> > userspace interfaces, but the fact remains that allowing truncated
> > responses causes *filesystem corruption*.
> >
> > We know that the most well known FIBMAP callers are bootloaders, and we
> > know what they do with the information they get -- they use it to record
> > the block map of boot files. So if the IPL/grub/whatever installer
> > queries the boot file and the boot file is at block 12345678901 (a
> > 34-bit number), this interface truncates that to 3755744309 (a 32-bit
> > number) and that's where the bootloader will think its boot files are.
> > The installation succeeds, the user reboots and *kaboom* the system no
> > longer boots because the contents of block 3755744309 is not a bootloader.
> >
> > Worse yet, grub1 used FIBMAP data to record the location of the grub
> > environment file and installed itself between the MBR and the start of
> > partition 1. If the environment file is at offset 1234578901, grub will
> > write status data to its environment file (which it thinks is at
> > 3755744309) and *KABOOM* we've just destroyed whatever was in that
> > block.
> >
> > Far better for the bootloader installation script to hit an error and
> > force the admin to deal with the situation than for the system to become
> > unbootable. That's *why* the (newer) iomap bmap implementation does not
> > return truncated mappings, even though the classic implementation does.
> >
> > The classic code returning truncated results is a broken behavior.
>
> How long as it been broken for?
Probably since the beginning (ext2).
> And if we do fix it, I'd just like for
> a nice commit lot describing potential risks of not applying it. *If*
> the issue exists as-is today, the above contains a lot of information
> for addressing potential issues, even if theoretical.
I think a lot of the filesystems avoid the problem either by not
supporting > INT_MAX blocks in the first place or by detecting the
truncation in the fs-specific ->bmap method, so that might be why we
haven't been deluged by corruption reports.
--D
> Luis
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-06 22:41 ` Luis Chamberlain
2019-08-07 14:42 ` Darrick J. Wong
@ 2019-08-08 7:12 ` Carlos Maiolino
2019-08-08 18:53 ` Andreas Dilger
1 sibling, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-08 7:12 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Darrick J. Wong, linux-fsdevel, hch, adilger, jaegeuk, miklos,
rpeterso, linux-xfs
> >
> > > Maybe I am not seeing something or having a different thinking you have, but
> > > this is the behavior we have now, without my patches. And we can't really change
> > > it; the user view of this implementation.
> > > That's why I didn't try to change the result, so the truncation still happens.
> >
> > I understand that we're not generally supposed to change existing
> > userspace interfaces, but the fact remains that allowing truncated
> > responses causes *filesystem corruption*.
> >
> > We know that the most well known FIBMAP callers are bootloaders, and we
> > know what they do with the information they get -- they use it to record
> > the block map of boot files. So if the IPL/grub/whatever installer
> > queries the boot file and the boot file is at block 12345678901 (a
> > 34-bit number), this interface truncates that to 3755744309 (a 32-bit
> > number) and that's where the bootloader will think its boot files are.
> > The installation succeeds, the user reboots and *kaboom* the system no
> > longer boots because the contents of block 3755744309 is not a bootloader.
> >
> > Worse yet, grub1 used FIBMAP data to record the location of the grub
> > environment file and installed itself between the MBR and the start of
> > partition 1. If the environment file is at offset 1234578901, grub will
> > write status data to its environment file (which it thinks is at
> > 3755744309) and *KABOOM* we've just destroyed whatever was in that
> > block.
> >
> > Far better for the bootloader installation script to hit an error and
> > force the admin to deal with the situation than for the system to become
> > unbootable. That's *why* the (newer) iomap bmap implementation does not
> > return truncated mappings, even though the classic implementation does.
> >
> > The classic code returning truncated results is a broken behavior.
>
> How long as it been broken for? And if we do fix it, I'd just like for
> a nice commit lot describing potential risks of not applying it. *If*
> the issue exists as-is today, the above contains a lot of information
> for addressing potential issues, even if theoretical.
>
It's broken since forever. This has always been the FIBMAP behavior.
> Luis
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-06 14:48 ` Darrick J. Wong
@ 2019-08-08 7:17 ` Carlos Maiolino
0 siblings, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-08 7:17 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Christoph Hellwig, linux-fsdevel, adilger, jaegeuk, miklos,
rpeterso, linux-xfs
On Tue, Aug 06, 2019 at 07:48:00AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 06, 2019 at 02:07:24PM +0200, Carlos Maiolino wrote:
> > On Tue, Aug 06, 2019 at 07:38:40AM +0200, Christoph Hellwig wrote:
> > > On Mon, Aug 05, 2019 at 08:12:58AM -0700, Darrick J. Wong wrote:
> > > > > returned. And IIRC, iomap is the only interface now that cares about issuing a
> > > > > warning.
> > > > >
> > > > > I think the *best* we could do here, is to make the new bmap() to issue the same
> > > > > kind of WARN() iomap does, but we can't really change the end result.
> > > >
> > > > I'd rather we break legacy code than corrupt filesystems.
> > >
> >
> > Yes, I have the same feeling, but this patchset does not have the goal to fix
> > the broken api.
> >
> > > This particular patch should keep existing behavior as is, as the intent
> > > is to not change functionality. Throwing in another patch to have saner
> > > error behavior now that we have a saner in-kernel interface that cleary
> > > documents what it is breaking and why on the other hand sounds like a
> > > very good idea.
> >
> > I totally agree here, and to be honest, I think such change should be in a
> > different patchset rather than a new patch in this series. I can do it for sure,
> > but this discussion IMHO should be done not only here in linux-fsdevel, but also
> > in linux-api, which well, I don't think cc'ing this whole patchset there will do
> > any good other than keep the change discussion more complicated than it should
> > be. I'd rather finish the design and implementation of this patchset, and I'll
> > follow-up it, once it's all set, with a new patch to change the truncation
> > behavior, it will make the discussion way easier than mixing up subjects. What
> > you guys think?
>
> I probably would've fixed the truncation behavior in the old code and
> based the fiemap-fibmap conversion on that so that anyone who wants to
> backport the behavior change to an old kernel has an easier time of it.
>
Well, another problem in fixing it in the old code, is that bmap() can't
properly return errors :P
After this patchset, bmap() will be able to return errors, so we can easily fix
it, once we won't need to 'guess' what a zero return mean from bmap()
> But afterwards probably works just as well since I don't feel like tying
> ourselves in more knots over an old interface. ;)
>
> --D
>
> > Cheers
> >
> >
> > --
> > Carlos
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-08 7:12 ` Carlos Maiolino
@ 2019-08-08 18:53 ` Andreas Dilger
2019-08-19 10:10 ` Carlos Maiolino
0 siblings, 1 reply; 47+ messages in thread
From: Andreas Dilger @ 2019-08-08 18:53 UTC (permalink / raw)
To: Carlos Maiolino
Cc: Luis Chamberlain, Darrick J. Wong, Linux FS-devel Mailing List,
Christoph Hellwig, Jaegeuk Kim, Miklos Szeredi, rpeterso,
linux-xfs
[-- Attachment #1: Type: text/plain, Size: 2847 bytes --]
On Aug 8, 2019, at 1:12 AM, Carlos Maiolino <cmaiolino@redhat.com> wrote:
>
>>>
>>>> Maybe I am not seeing something or having a different thinking you have, but
>>>> this is the behavior we have now, without my patches. And we can't really change
>>>> it; the user view of this implementation.
>>>> That's why I didn't try to change the result, so the truncation still happens.
>>>
>>> I understand that we're not generally supposed to change existing
>>> userspace interfaces, but the fact remains that allowing truncated
>>> responses causes *filesystem corruption*.
>>>
>>> We know that the most well known FIBMAP callers are bootloaders, and we
>>> know what they do with the information they get -- they use it to record
>>> the block map of boot files. So if the IPL/grub/whatever installer
>>> queries the boot file and the boot file is at block 12345678901 (a
>>> 34-bit number), this interface truncates that to 3755744309 (a 32-bit
>>> number) and that's where the bootloader will think its boot files are.
>>> The installation succeeds, the user reboots and *kaboom* the system no
>>> longer boots because the contents of block 3755744309 is not a bootloader.
>>>
>>> Worse yet, grub1 used FIBMAP data to record the location of the grub
>>> environment file and installed itself between the MBR and the start of
>>> partition 1. If the environment file is at offset 1234578901, grub will
>>> write status data to its environment file (which it thinks is at
>>> 3755744309) and *KABOOM* we've just destroyed whatever was in that
>>> block.
>>>
>>> Far better for the bootloader installation script to hit an error and
>>> force the admin to deal with the situation than for the system to become
>>> unbootable. That's *why* the (newer) iomap bmap implementation does not
>>> return truncated mappings, even though the classic implementation does.
>>>
>>> The classic code returning truncated results is a broken behavior.
>>
>> How long as it been broken for? And if we do fix it, I'd just like for
>> a nice commit lot describing potential risks of not applying it. *If*
>> the issue exists as-is today, the above contains a lot of information
>> for addressing potential issues, even if theoretical.
>>
>
> It's broken since forever. This has always been the FIBMAP behavior.
It's been broken since forever, but only for filesystems larger than 4TB or
16TB (2^32 blocks), which are only becoming commonplace for root disks recently.
Also, doesn't LILO have a limit on the location of the kernel image, in the
first 1GB or similar?
So maybe this is not an issue that FIBMAP users ever hit in practise anyway,
but I agree that it doesn't make sense to return bad data (32-bit wrapped block
numbers) and 0 should be returned in such cases.
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-08 18:53 ` Andreas Dilger
@ 2019-08-19 10:10 ` Carlos Maiolino
0 siblings, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-19 10:10 UTC (permalink / raw)
To: Andreas Dilger
Cc: Luis Chamberlain, Darrick J. Wong, Linux FS-devel Mailing List,
Christoph Hellwig, Jaegeuk Kim, Miklos Szeredi, rpeterso,
linux-xfs
Meh... Sorry andreas, your reply became disconnected from the thread, and I
think I didn't reply.
On Thu, Aug 08, 2019 at 12:53:25PM -0600, Andreas Dilger wrote:
> On Aug 8, 2019, at 1:12 AM, Carlos Maiolino <cmaiolino@redhat.com> wrote:
> >
> >>>
> >>>> Maybe I am not seeing something or having a different thinking you have, but
> >>>> this is the behavior we have now, without my patches. And we can't really change
> >>>> it; the user view of this implementation.
> >>>> That's why I didn't try to change the result, so the truncation still happens.
> >>>
> >>> I understand that we're not generally supposed to change existing
> >>> userspace interfaces, but the fact remains that allowing truncated
> >>> responses causes *filesystem corruption*.
> >>>
> >>> We know that the most well known FIBMAP callers are bootloaders, and we
> >>> know what they do with the information they get -- they use it to record
> >>> the block map of boot files. So if the IPL/grub/whatever installer
> >>> queries the boot file and the boot file is at block 12345678901 (a
> >>> 34-bit number), this interface truncates that to 3755744309 (a 32-bit
> >>> number) and that's where the bootloader will think its boot files are.
> >>> The installation succeeds, the user reboots and *kaboom* the system no
> >>> longer boots because the contents of block 3755744309 is not a bootloader.
> >>>
> >>> Worse yet, grub1 used FIBMAP data to record the location of the grub
> >>> environment file and installed itself between the MBR and the start of
> >>> partition 1. If the environment file is at offset 1234578901, grub will
> >>> write status data to its environment file (which it thinks is at
> >>> 3755744309) and *KABOOM* we've just destroyed whatever was in that
> >>> block.
> >>>
> >>> Far better for the bootloader installation script to hit an error and
> >>> force the admin to deal with the situation than for the system to become
> >>> unbootable. That's *why* the (newer) iomap bmap implementation does not
> >>> return truncated mappings, even though the classic implementation does.
> >>>
> >>> The classic code returning truncated results is a broken behavior.
> >>
> >> How long as it been broken for? And if we do fix it, I'd just like for
> >> a nice commit lot describing potential risks of not applying it. *If*
> >> the issue exists as-is today, the above contains a lot of information
> >> for addressing potential issues, even if theoretical.
> >>
> >
> > It's broken since forever. This has always been the FIBMAP behavior.
>
> It's been broken since forever, but only for filesystems larger than 4TB or
> 16TB (2^32 blocks), which are only becoming commonplace for root disks recently.
> Also, doesn't LILO have a limit on the location of the kernel image, in the
> first 1GB or similar?
>
> So maybe this is not an issue that FIBMAP users ever hit in practise anyway,
> but I agree that it doesn't make sense to return bad data (32-bit wrapped block
> numbers) and 0 should be returned in such cases.
>
Thanks for the input, but TBH I don't use LILO for a long time, and I don't
remember exactly how it works.
Anyway, I have 2 bugs to fix in this code, after I can get this series in, one
is the overflow we'll probably need kernel-api approval, and another one is the
acceptance of negative values into FIBMAP, which we have no protection at all.
I'll fix both once I can get the main series in.
Cheers
>
> Cheers, Andreas
>
>
>
>
>
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-09-11 13:43 [PATCH 0/9 V6] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
@ 2019-09-11 13:43 ` Carlos Maiolino
0 siblings, 0 replies; 47+ messages in thread
From: Carlos Maiolino @ 2019-09-11 13:43 UTC (permalink / raw)
To: linux-fsdevel; +Cc: hch, adilger, darrick.wong, linux-xfs
Now we have the possibility of proper error return in bmap, use bmap()
function in ioctl_fibmap() instead of calling ->bmap method directly.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
Changelog:
V6:
- Add a dummy bmap() definition so build does not break if
CONFIG_BLOCK is not set
Reported-by: kbuild test robot <lkp@intel.com>
V4:
- Ensure ioctl_fibmap() returns 0 in case of error returned from
bmap(). Otherwise we'll be changing the user interface (which
returns 0 in case of error)
V3:
- Rename usr_blk to ur_block
V2:
- Use a local sector_t variable to asign the block number
instead of using direct casting.
fs/ioctl.c | 29 +++++++++++++++++++----------
include/linux/fs.h | 7 +++++++
2 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index fef3a6bf7c78..6b589c873bc2 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -53,19 +53,28 @@ EXPORT_SYMBOL(vfs_ioctl);
static int ioctl_fibmap(struct file *filp, int __user *p)
{
- struct address_space *mapping = filp->f_mapping;
- int res, block;
+ struct inode *inode = file_inode(filp);
+ int error, ur_block;
+ sector_t block;
- /* do we support this mess? */
- if (!mapping->a_ops->bmap)
- return -EINVAL;
if (!capable(CAP_SYS_RAWIO))
return -EPERM;
- res = get_user(block, p);
- if (res)
- return res;
- res = mapping->a_ops->bmap(mapping, block);
- return put_user(res, p);
+
+ error = get_user(ur_block, p);
+ if (error)
+ return error;
+
+ block = ur_block;
+ error = bmap(inode, &block);
+
+ if (error)
+ ur_block = 0;
+ else
+ ur_block = block;
+
+ error = put_user(ur_block, p);
+
+ return error;
}
/**
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d787ceefc937..f1fbd8298ca4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2859,9 +2859,16 @@ static inline ssize_t generic_write_sync(struct kiocb *iocb, ssize_t count)
extern void emergency_sync(void);
extern void emergency_remount(void);
+
#ifdef CONFIG_BLOCK
extern int bmap(struct inode *, sector_t *);
+#else
+static inline int bmap(struct inode *, sector_t *)
+{
+ return -EINVAL;
+}
#endif
+
extern int notify_change(struct dentry *, struct iattr *, struct inode **);
extern int inode_permission(struct inode *, int);
extern int generic_permission(struct inode *, int);
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-14 11:01 ` Carlos Maiolino
@ 2019-08-14 11:08 ` Christoph Hellwig
0 siblings, 0 replies; 47+ messages in thread
From: Christoph Hellwig @ 2019-08-14 11:08 UTC (permalink / raw)
To: linux-fsdevel, hch, adilger, jaegeuk, darrick.wong, miklos,
rpeterso, linux-xfs
On Wed, Aug 14, 2019 at 01:01:50PM +0200, Carlos Maiolino wrote:
> Hey folks,
>
> > All errors (new ones prefixed by >>):
> >
> > fs/ioctl.c: In function 'ioctl_fibmap':
> > >> fs/ioctl.c:68:10: error: implicit declaration of function 'bmap'; did you mean 'kmap'? [-Werror=implicit-function-declaration]
>
> Any of you guys may have a better idea on how to fix this?
>
> Essentially, this happens when CONFIG_BLOCK is not set, and although I don't
> really see a hard requirement to have bmap() exported only when CONFIG_BLOCK is
> set, at the same time, I don't see use for bmap() if CONFIG_BLOCK is not set.
>
> So, I'm in a kind of a chicken-egg problem.
>
> I am considering to just remove the #ifdef CONFIG_BLOCK / #endif from the bmap()
> declaration. This will fix the warning, and I don't see any side effects. What
> you guys think?
Just provide an inline !CONFIG_BLOCK stub for bmap() in fs.h that always
returns -EINVAL.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-08 20:38 ` kbuild test robot
@ 2019-08-14 11:01 ` Carlos Maiolino
2019-08-14 11:08 ` Christoph Hellwig
0 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-14 11:01 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
Hey folks,
> All errors (new ones prefixed by >>):
>
> fs/ioctl.c: In function 'ioctl_fibmap':
> >> fs/ioctl.c:68:10: error: implicit declaration of function 'bmap'; did you mean 'kmap'? [-Werror=implicit-function-declaration]
Any of you guys may have a better idea on how to fix this?
Essentially, this happens when CONFIG_BLOCK is not set, and although I don't
really see a hard requirement to have bmap() exported only when CONFIG_BLOCK is
set, at the same time, I don't see use for bmap() if CONFIG_BLOCK is not set.
So, I'm in a kind of a chicken-egg problem.
I am considering to just remove the #ifdef CONFIG_BLOCK / #endif from the bmap()
declaration. This will fix the warning, and I don't see any side effects. What
you guys think?
> error = bmap(inode, &block);
> ^~~~
> kmap
> cc1: some warnings being treated as errors
>
> vim +68 fs/ioctl.c
>
> 53
> 54 static int ioctl_fibmap(struct file *filp, int __user *p)
> 55 {
> 56 struct inode *inode = file_inode(filp);
> 57 int error, ur_block;
> 58 sector_t block;
> 59
> 60 if (!capable(CAP_SYS_RAWIO))
> 61 return -EPERM;
> 62
> 63 error = get_user(ur_block, p);
> 64 if (error)
> 65 return error;
> 66
> 67 block = ur_block;
> > 68 error = bmap(inode, &block);
> 69
> 70 if (error)
> 71 ur_block = 0;
> 72 else
> 73 ur_block = block;
> 74
> 75 error = put_user(ur_block, p);
> 76
> 77 return error;
> 78 }
> 79
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
Carlos
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-08 8:27 ` [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap Carlos Maiolino
@ 2019-08-08 20:38 ` kbuild test robot
2019-08-14 11:01 ` Carlos Maiolino
0 siblings, 1 reply; 47+ messages in thread
From: kbuild test robot @ 2019-08-08 20:38 UTC (permalink / raw)
To: Carlos Maiolino
Cc: kbuild-all, linux-fsdevel, hch, adilger, jaegeuk, darrick.wong,
miklos, rpeterso, linux-xfs
[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]
Hi Carlos,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc3 next-20190808]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Carlos-Maiolino/New-fiemap-infrastructure-and-bmap-removal/20190808-221354
config: sh-allnoconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
fs/ioctl.c: In function 'ioctl_fibmap':
>> fs/ioctl.c:68:10: error: implicit declaration of function 'bmap'; did you mean 'kmap'? [-Werror=implicit-function-declaration]
error = bmap(inode, &block);
^~~~
kmap
cc1: some warnings being treated as errors
vim +68 fs/ioctl.c
53
54 static int ioctl_fibmap(struct file *filp, int __user *p)
55 {
56 struct inode *inode = file_inode(filp);
57 int error, ur_block;
58 sector_t block;
59
60 if (!capable(CAP_SYS_RAWIO))
61 return -EPERM;
62
63 error = get_user(ur_block, p);
64 if (error)
65 return error;
66
67 block = ur_block;
> 68 error = bmap(inode, &block);
69
70 if (error)
71 ur_block = 0;
72 else
73 ur_block = block;
74
75 error = put_user(ur_block, p);
76
77 return error;
78 }
79
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 5785 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
2019-08-08 8:27 [PATCH 0/9 V5] " Carlos Maiolino
@ 2019-08-08 8:27 ` Carlos Maiolino
2019-08-08 20:38 ` kbuild test robot
0 siblings, 1 reply; 47+ messages in thread
From: Carlos Maiolino @ 2019-08-08 8:27 UTC (permalink / raw)
To: linux-fsdevel
Cc: hch, adilger, jaegeuk, darrick.wong, miklos, rpeterso, linux-xfs
Now we have the possibility of proper error return in bmap, use bmap()
function in ioctl_fibmap() instead of calling ->bmap method directly.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
Changelog:
V4:
- Ensure ioctl_fibmap() returns 0 in case of error returned from
bmap(). Otherwise we'll be changing the user interface (which
returns 0 in case of error)
V3:
- Rename usr_blk to ur_block
V2:
- Use a local sector_t variable to asign the block number
instead of using direct casting.
fs/ioctl.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index fef3a6bf7c78..6b589c873bc2 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -53,19 +53,28 @@ EXPORT_SYMBOL(vfs_ioctl);
static int ioctl_fibmap(struct file *filp, int __user *p)
{
- struct address_space *mapping = filp->f_mapping;
- int res, block;
+ struct inode *inode = file_inode(filp);
+ int error, ur_block;
+ sector_t block;
- /* do we support this mess? */
- if (!mapping->a_ops->bmap)
- return -EINVAL;
if (!capable(CAP_SYS_RAWIO))
return -EPERM;
- res = get_user(block, p);
- if (res)
- return res;
- res = mapping->a_ops->bmap(mapping, block);
- return put_user(res, p);
+
+ error = get_user(ur_block, p);
+ if (error)
+ return error;
+
+ block = ur_block;
+ error = bmap(inode, &block);
+
+ if (error)
+ ur_block = 0;
+ else
+ ur_block = block;
+
+ error = put_user(ur_block, p);
+
+ return error;
}
/**
--
2.20.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
end of thread, other threads:[~2019-09-11 13:43 UTC | newest]
Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
2019-07-31 14:12 ` [PATCH 1/9] fs: Enable bmap() function to properly return errors Carlos Maiolino
2019-07-31 14:12 ` [PATCH 2/9] cachefiles: drop direct usage of ->bmap method Carlos Maiolino
2019-07-31 14:12 ` [PATCH 3/9] ecryptfs: drop direct calls to ->bmap Carlos Maiolino
2019-07-31 14:12 ` [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap Carlos Maiolino
2019-07-31 23:12 ` Darrick J. Wong
2019-08-02 9:19 ` Carlos Maiolino
2019-08-02 15:14 ` Darrick J. Wong
2019-08-05 10:27 ` Carlos Maiolino
2019-08-05 15:12 ` Darrick J. Wong
2019-08-06 5:38 ` Christoph Hellwig
2019-08-06 12:07 ` Carlos Maiolino
2019-08-06 14:48 ` Darrick J. Wong
2019-08-08 7:17 ` Carlos Maiolino
2019-08-06 12:02 ` Carlos Maiolino
2019-08-06 22:41 ` Luis Chamberlain
2019-08-07 14:42 ` Darrick J. Wong
2019-08-08 7:12 ` Carlos Maiolino
2019-08-08 18:53 ` Andreas Dilger
2019-08-19 10:10 ` Carlos Maiolino
2019-07-31 14:12 ` [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info Carlos Maiolino
2019-07-31 23:28 ` Darrick J. Wong
2019-08-02 9:51 ` Carlos Maiolino
2019-08-02 15:15 ` Darrick J. Wong
2019-08-05 9:40 ` Carlos Maiolino
2019-08-06 5:39 ` Christoph Hellwig
2019-07-31 14:12 ` [PATCH 6/9] iomap: Remove length and start fields from iomap_fiemap Carlos Maiolino
2019-07-31 23:24 ` Darrick J. Wong
2019-07-31 14:12 ` [PATCH 7/9] fiemap: Use a callback to fill fiemap extents Carlos Maiolino
2019-07-31 23:26 ` Darrick J. Wong
2019-07-31 14:12 ` [PATCH 8/9] Use FIEMAP for FIBMAP calls Carlos Maiolino
2019-07-31 23:22 ` Darrick J. Wong
2019-07-31 23:31 ` Darrick J. Wong
2019-08-02 13:52 ` Carlos Maiolino
2019-08-06 5:41 ` Christoph Hellwig
2019-08-02 13:48 ` Carlos Maiolino
2019-08-02 15:29 ` Darrick J. Wong
2019-08-05 10:38 ` Carlos Maiolino
2019-08-06 5:46 ` Christoph Hellwig
2019-07-31 14:12 ` [PATCH 9/9] xfs: Get rid of ->bmap Carlos Maiolino
2019-07-31 23:30 ` Darrick J. Wong
2019-08-02 10:20 ` [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
2019-08-08 8:27 [PATCH 0/9 V5] " Carlos Maiolino
2019-08-08 8:27 ` [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap Carlos Maiolino
2019-08-08 20:38 ` kbuild test robot
2019-08-14 11:01 ` Carlos Maiolino
2019-08-14 11:08 ` Christoph Hellwig
2019-09-11 13:43 [PATCH 0/9 V6] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
2019-09-11 13:43 ` [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap Carlos Maiolino
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).