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:

quoted

--- 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