Re: [PATCHv2 1/3] ext4: Refactor ext4_overwrite_io() to take ext4_map_blocks as argument
On Sat, Aug 22, 2020 at 05:04:35PM +0530, Ritesh Harjani wrote:
> Refactor ext4_overwrite_io() to take struct ext4_map_blocks
> as it's function argument with m_lblk and m_len filled
> from caller
>
> There should be no functionality change in this patch.
>
> Signed-off-by: Ritesh Harjani
> ---
> fs/ext4/file.c | 22 +++---
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 2a01e31a032c..84f73ed91af2 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -188,26 +188,22 @@ ext4_extending_io(struct inode *inode, loff_t offset,
> size_t len)
> }
>
> /* Is IO overwriting allocated and initialized blocks? */
> -static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
> +static bool ext4_overwrite_io(struct inode *inode, struct ext4_map_blocks
> *map)
> {
> - struct ext4_map_blocks map;
> unsigned int blkbits = inode->i_blkbits;
> - int err, blklen;ts
> + loff_t end = (map->m_lblk + map->m_len) << blkbits;
As Dan Carpenter has pointed out, we need to cast map->m_lblk to
loff_t, since m_lblk is 32 bits, and when this get shifted left by
blkbits, we could end up losing bits.
> - if (pos + len > i_size_read(inode))
> + if (end > i_size_read(inode))
> return false;
This transformation is not functionally identical.
The problem is that pos is not necessarily a multiple of the file
system blocksize.From below,
> + map.m_lblk = offset >> inode->i_blkbits;
> + map.m_len = EXT4_MAX_BLOCKS(count, offset, inode->i_blkbits);
So what previously was the starting offset of the overwrite, is now
offset shifted right by blkbits, and then shifted left back by blkbits.
So unless I'm missing something, this looks not quite right?
- Ted
Re: [PATCHv2 1/3] ext4: Refactor ext4_overwrite_io() to take ext4_map_blocks as argument
Hi Ritesh,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v5.9-rc1]
[cannot apply to ext4/dev next-20200821]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Ritesh-Harjani/Optimize-ext4-DAX-overwrites/20200822-193615
base:9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
config: i386-randconfig-m021-20200822 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
New smatch warnings:
fs/ext4/file.c:194 ext4_overwrite_io() warn: should '(map->m_lblk + map->m_len)
<< blkbits' be a 64 bit type?
Old smatch warnings:
include/linux/fs.h:867 i_size_write() warn: statement has no effect 31
fs/ext4/file.c:585 ext4_dio_write_iter() warn: inconsistent returns
'inode->i_rwsem'.
#
https://github.com/0day-ci/linux/commit/5d171d1d87ee0aca0a992b6843d154b41466e5e5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Ritesh-Harjani/Optimize-ext4-DAX-overwrites/20200822-193615
git checkout 5d171d1d87ee0aca0a992b6843d154b41466e5e5
vim +194 fs/ext4/file.c
e9e3bcecf44c04 Eric Sandeen 2011-02-12 189
213bcd9ccbf04b Jan Kara 2016-11-20 190 /* Is IO overwriting allocated
and initialized blocks? */
5d171d1d87ee0a Ritesh Harjani 2020-08-22 191 static bool
ext4_overwrite_io(struct inode *inode, struct ext4_map_blocks *map)
213bcd9ccbf04b Jan Kara 2016-11-20 192 {
213bcd9ccbf04b Jan Kara 2016-11-20 193 unsigned int blkbits =
inode->i_blkbits;
5d171d1d87ee0a Ritesh Harjani 2020-08-22 @194 loff_t end = (map->m_lblk +
map->m_len) << blkbits;
^
potential shift wrap?
5d171d1d87ee0a Ritesh Harjani 2020-08-22 195 int err, blklen = map->m_len;
213bcd9ccbf04b Jan Kara 2016-11-20 196
5d171d1d87ee0a Ritesh Harjani 2020-08-22 197 if (end > i_size_read(inode))
213bcd9ccbf04b Jan Kara 2016-11-20 198 return false;
213bcd9ccbf04b Jan Kara 2016-11-20 199
5d171d1d87ee0a Ritesh Harjani 2020-08-22 200 err = ext4_map_blocks(NULL,
inode, map, 0);
213bcd9ccbf04b Jan Kara 2016-11-20 201 /*
213bcd9ccbf04b Jan Kara 2016-11-20 202* 'err==len' means that all of
the blocks have been preallocated,
213bcd9ccbf04b Jan Kara 2016-11-20 203* regardless of whether they
have been initialized or not. To exclude
213bcd9ccbf04b Jan Kara 2016-11-20 204* unwritten extents, we need
to check m_flags.
213bcd9ccbf04b Jan Kara 2016-11-20 205*/
5d171d1d87ee0a Ritesh Harjani 2020-08-22 206 return err == blklen &&
(map->m_flags & EXT4_MAP_MAPPED);
213bcd9ccbf04b Jan Kara 2016-11-20 207 }
213bcd9ccbf04b Jan Kara 2016-11-20 208
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
[PATCHv2 1/3] ext4: Refactor ext4_overwrite_io() to take ext4_map_blocks as argument
Refactor ext4_overwrite_io() to take struct ext4_map_blocks
as it's function argument with m_lblk and m_len filled
from caller
There should be no functionality change in this patch.
Signed-off-by: Ritesh Harjani
---
fs/ext4/file.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 2a01e31a032c..84f73ed91af2 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -188,26 +188,22 @@ ext4_extending_io(struct inode *inode, loff_t offset,
size_t len)
}
/* Is IO overwriting allocated and initialized blocks? */
-static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
+static bool ext4_overwrite_io(struct inode *inode, struct ext4_map_blocks *map)
{
- struct ext4_map_blocks map;
unsigned int blkbits = inode->i_blkbits;
- int err, blklen;
+ loff_t end = (map->m_lblk + map->m_len) << blkbits;
+ int err, blklen = map->m_len;
- if (pos + len > i_size_read(inode))
+ if (end > i_size_read(inode))
return false;
- map.m_lblk = pos >> blkbits;
- map.m_len = EXT4_MAX_BLOCKS(len, pos, blkbits);
- blklen = map.m_len;
-
- err = ext4_map_blocks(NULL, inode, , 0);
+ err = ext4_map_blocks(NULL, inode, map, 0);
/*
* 'err==len' means that all of the blocks have been preallocated,
* regardless of whether they have been initialized or not. To exclude
* unwritten extents, we need to check m_flags.
*/
- return err == blklen && (map.m_flags & EXT4_MAP_MAPPED);
+ return err == blklen && (map->m_flags & EXT4_MAP_MAPPED);
}
static ssize_t ext4_generic_write_checks(struct kiocb *iocb,
@@ -407,6 +403,7 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb,
struct iov_iter *from,
{
struct file *file = iocb->ki_filp;
struct inode *inode = file_inode(file);
+ struct ext4_map_blocks map;
loff_t offset;
size_t count;
ssize_t ret;
@@ -420,6 +417,9 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb,
struct iov_iter *from,
count = ret;
if (ext4_extending_io(inode, offset, count))
*extend = true;
+
+ map.m_lblk = offset >> inode->i_blkbits;
+ map.m_len = EXT4_MAX_BLOCKS(count, offset, inode->i_blkbits);
/*
* Determine whether the IO operation will overwrite allocated
* and initialized blocks.
@@ -427,7 +427,7 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb,
struct iov_iter *from,
* in file_modified().
*/
if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
-!ext4_overwrite_io(inode, offset, count))) {
+!ext4_overwrite_io(inode, ))) {
inode_unlock_shared(inode);
*ilock_shared = false;
inode_lock(inode);
--
2.25.4
