From: Ilya Dryomov <idryomov@gmail.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Chris Mason <chris.mason@oracle.com>,
linux-btrfs@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [patch] Btrfs: fix bitwise vs logical condition
Date: Fri, 20 Jan 2012 16:00:50 +0000 [thread overview]
Message-ID: <20120120160050.GA25145@zambezi.lan> (raw)
In-Reply-To: <20120120152129.GO3356@mwanda>
On Fri, Jan 20, 2012 at 06:21:29PM +0300, Dan Carpenter wrote:
> On Fri, Jan 20, 2012 at 04:24:37PM +0200, Ilya Dryomov wrote:
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -2375,12 +2375,11 @@ static int should_balance_chunk(struct btrfs_root *root,
> > struct btrfs_balance_control *bctl = root->fs_info->balance_ctl;
> > struct btrfs_balance_args *bargs = NULL;
> > u64 chunk_type = btrfs_chunk_type(leaf, chunk);
> > + u64 mask = chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK;
> >
> > /* type filter */
> > - if (!((chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) &
> > - (bctl->flags & BTRFS_BALANCE_TYPE_MASK))) {
> > + if (((bctl->flags & BTRFS_BALANCE_TYPE_MASK) & mask) != mask)
>
> I don't know if it matters, but semantically this is not
> equivalent to the original. If mask has no flags set then this will
> pass. This says that every flag in mask but has to be set in
> ->flags but in the original code, only one needed to be.
Yeah, that's why I said "we can strengthen that check".
>
> The original code was equivalent to:
> if (!(chunk_type & bctl->flags & BTRFS_BLOCK_GROUP_TYPE_MASK &
> BTRFS_BALANCE_TYPE_MASK)) {...
>
> It's weird that we have BTRFS_BLOCK_GROUP_TYPE_MASK and
> BTRFS_BALANCE_TYPE_MASK which are the same except that the bitfields
> have been renamed. Can't we just reuse the first definition?
>
> But really, if this isn't a bug, then I don't care. The original is
> fine, or whatever you choose.
This is not a bug.
Thanks,
Ilya
WARNING: multiple messages have this Message-ID (diff)
From: Ilya Dryomov <idryomov@gmail.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Chris Mason <chris.mason@oracle.com>,
linux-btrfs@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [patch] Btrfs: fix bitwise vs logical condition
Date: Fri, 20 Jan 2012 18:00:50 +0200 [thread overview]
Message-ID: <20120120160050.GA25145@zambezi.lan> (raw)
In-Reply-To: <20120120152129.GO3356@mwanda>
On Fri, Jan 20, 2012 at 06:21:29PM +0300, Dan Carpenter wrote:
> On Fri, Jan 20, 2012 at 04:24:37PM +0200, Ilya Dryomov wrote:
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -2375,12 +2375,11 @@ static int should_balance_chunk(struct btrfs_root *root,
> > struct btrfs_balance_control *bctl = root->fs_info->balance_ctl;
> > struct btrfs_balance_args *bargs = NULL;
> > u64 chunk_type = btrfs_chunk_type(leaf, chunk);
> > + u64 mask = chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK;
> >
> > /* type filter */
> > - if (!((chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) &
> > - (bctl->flags & BTRFS_BALANCE_TYPE_MASK))) {
> > + if (((bctl->flags & BTRFS_BALANCE_TYPE_MASK) & mask) != mask)
>
> I don't know if it matters, but semantically this is not
> equivalent to the original. If mask has no flags set then this will
> pass. This says that every flag in mask but has to be set in
> ->flags but in the original code, only one needed to be.
Yeah, that's why I said "we can strengthen that check".
>
> The original code was equivalent to:
> if (!(chunk_type & bctl->flags & BTRFS_BLOCK_GROUP_TYPE_MASK &
> BTRFS_BALANCE_TYPE_MASK)) {...
>
> It's weird that we have BTRFS_BLOCK_GROUP_TYPE_MASK and
> BTRFS_BALANCE_TYPE_MASK which are the same except that the bitfields
> have been renamed. Can't we just reuse the first definition?
>
> But really, if this isn't a bug, then I don't care. The original is
> fine, or whatever you choose.
This is not a bug.
Thanks,
Ilya
next prev parent reply other threads:[~2012-01-20 16:00 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-01-20 7:54 [patch] Btrfs: fix bitwise vs logical condition Dan Carpenter
2012-01-20 7:54 ` Dan Carpenter
2012-01-20 14:24 ` Ilya Dryomov
2012-01-20 14:24 ` Ilya Dryomov
2012-01-20 15:21 ` Dan Carpenter
2012-01-20 15:21 ` Dan Carpenter
2012-01-20 16:00 ` Ilya Dryomov [this message]
2012-01-20 16:00 ` Ilya Dryomov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120120160050.GA25145@zambezi.lan \
--to=idryomov@gmail.com \
--cc=chris.mason@oracle.com \
--cc=dan.carpenter@oracle.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.