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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
