| Message ID | 56fc8d3802c578d27d49270600946a0737cef119.1582702694.git.riteshh@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: bmap & fiemap conversion to use iomap | expand |
On Wed 26-02-20 15:27:06, Ritesh Harjani wrote: > For indirect block mapping if the i_block > max supported block in inode > then ext4_ind_map_blocks may return a -EIO error. But in case of fiemap > this could be a valid query to ext4_map_blocks. > So in case if !create then return 0. This also makes ext4_warning to > ext4_debug in ext4_block_to_path() for the same reason. > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> Hmm, won't it be cleaner to just handle this in ext4_iomap_begin_report()? We do trim map.m_len there anyway so it is only logical to trim it to proper value supported by the inode on-disk format... BTW, note we have sbi->s_bitmap_maxbytes value already computed in the superblock... Honza > --- > fs/ext4/indirect.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c > index 3a4ab70fe9e0..e1ab495dd900 100644 > --- a/fs/ext4/indirect.c > +++ b/fs/ext4/indirect.c > @@ -102,7 +102,11 @@ static int ext4_block_to_path(struct inode *inode, > offsets[n++] = i_block & (ptrs - 1); > final = ptrs; > } else { > - ext4_warning(inode->i_sb, "block %lu > max in inode %lu", > + /* > + * It's not yet an error to just query beyond max > + * block in inode. Fiemap callers may do so. > + */ > + ext4_debug("block %lu > max in inode %lu", > i_block + direct_blocks + > indirect_blocks + double_blocks, inode->i_ino); > } > @@ -537,8 +541,11 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode, > depth = ext4_block_to_path(inode, map->m_lblk, offsets, > &blocks_to_boundary); > > - if (depth == 0) > + if (depth == 0) { > + if (!(flags & EXT4_GET_BLOCKS_CREATE)) > + err = 0; > goto out; > + } > > partial = ext4_get_branch(inode, depth, offsets, chain, &err); > > -- > 2.21.0 >
On 2/26/20 6:09 PM, Jan Kara wrote: > On Wed 26-02-20 15:27:06, Ritesh Harjani wrote: >> For indirect block mapping if the i_block > max supported block in inode >> then ext4_ind_map_blocks may return a -EIO error. But in case of fiemap >> this could be a valid query to ext4_map_blocks. >> So in case if !create then return 0. This also makes ext4_warning to >> ext4_debug in ext4_block_to_path() for the same reason. >> >> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> > > Hmm, won't it be cleaner to just handle this in ext4_iomap_begin_report()? > We do trim map.m_len there anyway so it is only logical to trim it to > proper value supported by the inode on-disk format... BTW, note we have > sbi->s_bitmap_maxbytes value already computed in the superblock... hmm. Yes, thanks for the pointers. Let me check this again. -ritesh > > Honza > >> --- >> fs/ext4/indirect.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c >> index 3a4ab70fe9e0..e1ab495dd900 100644 >> --- a/fs/ext4/indirect.c >> +++ b/fs/ext4/indirect.c >> @@ -102,7 +102,11 @@ static int ext4_block_to_path(struct inode *inode, >> offsets[n++] = i_block & (ptrs - 1); >> final = ptrs; >> } else { >> - ext4_warning(inode->i_sb, "block %lu > max in inode %lu", >> + /* >> + * It's not yet an error to just query beyond max >> + * block in inode. Fiemap callers may do so. >> + */ >> + ext4_debug("block %lu > max in inode %lu", >> i_block + direct_blocks + >> indirect_blocks + double_blocks, inode->i_ino); >> } >> @@ -537,8 +541,11 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode, >> depth = ext4_block_to_path(inode, map->m_lblk, offsets, >> &blocks_to_boundary); >> >> - if (depth == 0) >> + if (depth == 0) { >> + if (!(flags & EXT4_GET_BLOCKS_CREATE)) >> + err = 0; >> goto out; >> + } >> >> partial = ext4_get_branch(inode, depth, offsets, chain, &err); >> >> -- >> 2.21.0 >>
On Wed, Feb 26, 2020 at 03:27:06PM +0530, Ritesh Harjani wrote: > For indirect block mapping if the i_block > max supported block in inode > then ext4_ind_map_blocks may return a -EIO error. But in case of fiemap > this could be a valid query to ext4_map_blocks. > So in case if !create then return 0. This also makes ext4_warning to > ext4_debug in ext4_block_to_path() for the same reason. > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> > --- > fs/ext4/indirect.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c > index 3a4ab70fe9e0..e1ab495dd900 100644 > --- a/fs/ext4/indirect.c > +++ b/fs/ext4/indirect.c > @@ -102,7 +102,11 @@ static int ext4_block_to_path(struct inode *inode, > offsets[n++] = i_block & (ptrs - 1); > final = ptrs; > } else { > - ext4_warning(inode->i_sb, "block %lu > max in inode %lu", > + /* > + * It's not yet an error to just query beyond max > + * block in inode. Fiemap callers may do so. > + */ > + ext4_debug("block %lu > max in inode %lu", > i_block + direct_blocks + > indirect_blocks + double_blocks, inode->i_ino); Does that mean fiemap callers can spamflood dmesg with this message just by setting the query start range to a huge value? --D > } > @@ -537,8 +541,11 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode, > depth = ext4_block_to_path(inode, map->m_lblk, offsets, > &blocks_to_boundary); > > - if (depth == 0) > + if (depth == 0) { > + if (!(flags & EXT4_GET_BLOCKS_CREATE)) > + err = 0; > goto out; > + } > > partial = ext4_get_branch(inode, depth, offsets, chain, &err); > > -- > 2.21.0 >
On 2/26/20 9:41 PM, Darrick J. Wong wrote: > On Wed, Feb 26, 2020 at 03:27:06PM +0530, Ritesh Harjani wrote: >> For indirect block mapping if the i_block > max supported block in inode >> then ext4_ind_map_blocks may return a -EIO error. But in case of fiemap >> this could be a valid query to ext4_map_blocks. >> So in case if !create then return 0. This also makes ext4_warning to >> ext4_debug in ext4_block_to_path() for the same reason. >> >> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> >> --- >> fs/ext4/indirect.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c >> index 3a4ab70fe9e0..e1ab495dd900 100644 >> --- a/fs/ext4/indirect.c >> +++ b/fs/ext4/indirect.c >> @@ -102,7 +102,11 @@ static int ext4_block_to_path(struct inode *inode, >> offsets[n++] = i_block & (ptrs - 1); >> final = ptrs; >> } else { >> - ext4_warning(inode->i_sb, "block %lu > max in inode %lu", >> + /* >> + * It's not yet an error to just query beyond max >> + * block in inode. Fiemap callers may do so. >> + */ >> + ext4_debug("block %lu > max in inode %lu", >> i_block + direct_blocks + >> indirect_blocks + double_blocks, inode->i_ino); > > Does that mean fiemap callers can spamflood dmesg with this message just > by setting the query start range to a huge value? Not in the old implementation. But This could happen with indirect block mapping with new implementation in iomap (as there is no check in place before calling ext4_map_blocks()). Previously __generic_block_fiemap() used to not query beyond i_size_read(), so we were safe there. So yes now as Jan also suggested, will add a check in place in ext4_iomap_begin_report() itself, so that this flooding wont happen. Thanks for the review!! -ritesh > > --D > >> } >> @@ -537,8 +541,11 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode, >> depth = ext4_block_to_path(inode, map->m_lblk, offsets, >> &blocks_to_boundary); >> >> - if (depth == 0) >> + if (depth == 0) { >> + if (!(flags & EXT4_GET_BLOCKS_CREATE)) >> + err = 0; >> goto out; >> + } >> >> partial = ext4_get_branch(inode, depth, offsets, chain, &err); >> >> -- >> 2.21.0 >>
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c index 3a4ab70fe9e0..e1ab495dd900 100644 --- a/fs/ext4/indirect.c +++ b/fs/ext4/indirect.c @@ -102,7 +102,11 @@ static int ext4_block_to_path(struct inode *inode, offsets[n++] = i_block & (ptrs - 1); final = ptrs; } else { - ext4_warning(inode->i_sb, "block %lu > max in inode %lu", + /* + * It's not yet an error to just query beyond max + * block in inode. Fiemap callers may do so. + */ + ext4_debug("block %lu > max in inode %lu", i_block + direct_blocks + indirect_blocks + double_blocks, inode->i_ino); } @@ -537,8 +541,11 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode, depth = ext4_block_to_path(inode, map->m_lblk, offsets, &blocks_to_boundary); - if (depth == 0) + if (depth == 0) { + if (!(flags & EXT4_GET_BLOCKS_CREATE)) + err = 0; goto out; + } partial = ext4_get_branch(inode, depth, offsets, chain, &err);
For indirect block mapping if the i_block > max supported block in inode then ext4_ind_map_blocks may return a -EIO error. But in case of fiemap this could be a valid query to ext4_map_blocks. So in case if !create then return 0. This also makes ext4_warning to ext4_debug in ext4_block_to_path() for the same reason. Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> --- fs/ext4/indirect.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)