* [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents()
@ 2014-03-31 9:11 Lukas Czerner
2014-03-31 9:11 ` [PATCH 2/2] ext4: Remove unneeded test of ret variable Lukas Czerner
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Lukas Czerner @ 2014-03-31 9:11 UTC (permalink / raw)
To: linux-ext4; +Cc: dan.carpenter, Lukas Czerner
In commit 1f0e51771281 "ext4: Introduce FALLOC_FL_ZERO_RANGE flag
for fallocate" we've introduced wrong flag handling. Fix it.
Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
fs/ext4/extents.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 243a02e..491208c 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3644,13 +3644,13 @@ static int ext4_split_convert_extents(handle_t *handle,
ee_len = ext4_ext_get_actual_len(ex);
/* Convert to unwritten */
- if (flags | EXT4_GET_BLOCKS_CONVERT_UNWRITTEN) {
+ if (flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN) {
split_flag |= EXT4_EXT_DATA_VALID1;
/* Convert to initialized */
- } else if (flags | EXT4_GET_BLOCKS_CONVERT) {
+ } else if (flags & EXT4_GET_BLOCKS_CONVERT) {
split_flag |= ee_block + ee_len <= eof_block ?
EXT4_EXT_MAY_ZEROOUT : 0;
- split_flag |= (EXT4_EXT_MARK_UNINIT2 & EXT4_EXT_DATA_VALID2);
+ split_flag |= (EXT4_EXT_MARK_UNINIT2 | EXT4_EXT_DATA_VALID2);
}
flags |= EXT4_GET_BLOCKS_PRE_IO;
return ext4_split_extent(handle, inode, path, map, split_flag, flags);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] ext4: Remove unneeded test of ret variable
2014-03-31 9:11 [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents() Lukas Czerner
@ 2014-03-31 9:11 ` Lukas Czerner
2014-04-01 5:00 ` Theodore Ts'o
2014-03-31 14:12 ` [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents() Dmitry Monakhov
2014-04-01 4:57 ` Theodore Ts'o
2 siblings, 1 reply; 6+ messages in thread
From: Lukas Czerner @ 2014-03-31 9:11 UTC (permalink / raw)
To: linux-ext4; +Cc: dan.carpenter, Lukas Czerner
Currently in ext4_fallocate() and ext4_zero_range() we're testing ret
variable along with new_size. However in ext4_fallocate() we just tested
ret before and in ext4_zero_range() if will always be zero when we get
there so there is no need to test it in both cases.
Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
fs/ext4/extents.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 491208c..8c09e1d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4816,12 +4816,12 @@ static long ext4_zero_range(struct file *file, loff_t offset,
inode->i_mtime = inode->i_ctime = ext4_current_time(inode);
- if (!ret && new_size) {
+ if (new_size) {
if (new_size > i_size_read(inode))
i_size_write(inode, new_size);
if (new_size > EXT4_I(inode)->i_disksize)
ext4_update_i_disksize(inode, new_size);
- } else if (!ret && !new_size) {
+ } else {
/*
* Mark that we allocate beyond EOF so the subsequent truncate
* can proceed even if the new size is the same as i_size.
@@ -4923,14 +4923,14 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
tv = inode->i_ctime = ext4_current_time(inode);
- if (!ret && new_size) {
+ if (new_size) {
if (new_size > i_size_read(inode)) {
i_size_write(inode, new_size);
inode->i_mtime = tv;
}
if (new_size > EXT4_I(inode)->i_disksize)
ext4_update_i_disksize(inode, new_size);
- } else if (!ret && !new_size) {
+ } else {
/*
* Mark that we allocate beyond EOF so the subsequent truncate
* can proceed even if the new size is the same as i_size.
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents()
2014-03-31 9:11 [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents() Lukas Czerner
2014-03-31 9:11 ` [PATCH 2/2] ext4: Remove unneeded test of ret variable Lukas Czerner
@ 2014-03-31 14:12 ` Dmitry Monakhov
2014-03-31 14:19 ` Dan Carpenter
2014-04-01 4:57 ` Theodore Ts'o
2 siblings, 1 reply; 6+ messages in thread
From: Dmitry Monakhov @ 2014-03-31 14:12 UTC (permalink / raw)
To: Lukas Czerner, linux-ext4; +Cc: dan.carpenter, Lukas Czerner
On Mon, 31 Mar 2014 11:11:33 +0200, Lukas Czerner <lczerner@redhat.com> wrote:
> In commit 1f0e51771281 "ext4: Introduce FALLOC_FL_ZERO_RANGE flag
> for fallocate" we've introduced wrong flag handling. Fix it.
>
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
> fs/ext4/extents.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 243a02e..491208c 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3644,13 +3644,13 @@ static int ext4_split_convert_extents(handle_t *handle,
> ee_len = ext4_ext_get_actual_len(ex);
>
> /* Convert to unwritten */
> - if (flags | EXT4_GET_BLOCKS_CONVERT_UNWRITTEN) {
> + if (flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN) {
:). But how did you found this?
I think that this type of bugs should be caught by some semantics
analyzer? I've done simple test and sparse(1) owerlooked this,
Also I cant find specific rule for Coccinelle ( if (var | CONST)).
> split_flag |= EXT4_EXT_DATA_VALID1;
> /* Convert to initialized */
> - } else if (flags | EXT4_GET_BLOCKS_CONVERT) {
> + } else if (flags & EXT4_GET_BLOCKS_CONVERT) {
> split_flag |= ee_block + ee_len <= eof_block ?
> EXT4_EXT_MAY_ZEROOUT : 0;
> - split_flag |= (EXT4_EXT_MARK_UNINIT2 & EXT4_EXT_DATA_VALID2);
> + split_flag |= (EXT4_EXT_MARK_UNINIT2 | EXT4_EXT_DATA_VALID2);
> }
> flags |= EXT4_GET_BLOCKS_PRE_IO;
> return ext4_split_extent(handle, inode, path, map, split_flag, flags);
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents()
2014-03-31 14:12 ` [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents() Dmitry Monakhov
@ 2014-03-31 14:19 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2014-03-31 14:19 UTC (permalink / raw)
To: Dmitry Monakhov; +Cc: Lukas Czerner, linux-ext4
On Mon, Mar 31, 2014 at 06:12:39PM +0400, Dmitry Monakhov wrote:
> On Mon, 31 Mar 2014 11:11:33 +0200, Lukas Czerner <lczerner@redhat.com> wrote:
> > In commit 1f0e51771281 "ext4: Introduce FALLOC_FL_ZERO_RANGE flag
> > for fallocate" we've introduced wrong flag handling. Fix it.
> >
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> > fs/ext4/extents.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 243a02e..491208c 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -3644,13 +3644,13 @@ static int ext4_split_convert_extents(handle_t *handle,
> > ee_len = ext4_ext_get_actual_len(ex);
> >
> > /* Convert to unwritten */
> > - if (flags | EXT4_GET_BLOCKS_CONVERT_UNWRITTEN) {
> > + if (flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN) {
> :). But how did you found this?
> I think that this type of bugs should be caught by some semantics
> analyzer? I've done simple test and sparse(1) owerlooked this,
> Also I cant find specific rule for Coccinelle ( if (var | CONST)).
This one was found with a Smatch warning:
fs/ext4/extents.c:3647 ext4_split_convert_extents() warn: suspicious bitop condition
>
> > split_flag |= EXT4_EXT_DATA_VALID1;
> > /* Convert to initialized */
> > - } else if (flags | EXT4_GET_BLOCKS_CONVERT) {
> > + } else if (flags & EXT4_GET_BLOCKS_CONVERT) {
> > split_flag |= ee_block + ee_len <= eof_block ?
> > EXT4_EXT_MAY_ZEROOUT : 0;
> > - split_flag |= (EXT4_EXT_MARK_UNINIT2 & EXT4_EXT_DATA_VALID2);
> > + split_flag |= (EXT4_EXT_MARK_UNINIT2 | EXT4_EXT_DATA_VALID2);
I would like to push the Smatch warning for this one as well, but there
are too many places where these kinds of AND operations are valid.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents()
2014-03-31 9:11 [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents() Lukas Czerner
2014-03-31 9:11 ` [PATCH 2/2] ext4: Remove unneeded test of ret variable Lukas Czerner
2014-03-31 14:12 ` [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents() Dmitry Monakhov
@ 2014-04-01 4:57 ` Theodore Ts'o
2 siblings, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2014-04-01 4:57 UTC (permalink / raw)
To: Lukas Czerner; +Cc: linux-ext4, dan.carpenter
On Mon, Mar 31, 2014 at 11:11:33AM +0200, Lukas Czerner wrote:
> In commit 1f0e51771281 "ext4: Introduce FALLOC_FL_ZERO_RANGE flag
> for fallocate" we've introduced wrong flag handling. Fix it.
>
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Thanks, I've merged this into the base zero_range patch.
- Ted
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-04-01 5:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-31 9:11 [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents() Lukas Czerner
2014-03-31 9:11 ` [PATCH 2/2] ext4: Remove unneeded test of ret variable Lukas Czerner
2014-04-01 5:00 ` Theodore Ts'o
2014-03-31 14:12 ` [PATCH 1/2] ext4: Fix flag handling in ext4_split_convert_extents() Dmitry Monakhov
2014-03-31 14:19 ` Dan Carpenter
2014-04-01 4:57 ` Theodore Ts'o
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).