Re: [PATCH v2 0/5] define BTRFS_DEV_STATE

[David Sterba <dsterba@suse.cz>]

On Tue, Dec 05, 2017 at 09:20:38AM +0800, Anand Jain wrote:
> 
> 
> On 12/05/2017 04:28 AM, David Sterba wrote:
> > On Mon, Dec 04, 2017 at 12:54:51PM +0800, Anand Jain wrote:
> >> As of now device properties and states are being represented as int
> >> variable. So clean that up using bitwise operations. Also patches in
> >> the ML such as device failed state needs this cleanup as well.
> >>
> >> V2:
> >>   Accepts all comments from Nikolay.
> >>   Drops can_discard.
> >>   Adds BTRFS_DEV_STATE_REPLACE_TGT and BTRFS_DEV_STATE_FLUSH_SENT
> >>      patches.
> >>
> >> Anand Jain (5):
> >>    btrfs: cleanup device states define BTRFS_DEV_STATE_WRITEABLE
> >>    btrfs: cleanup device states define BTRFS_DEV_STATE_IN_FS_METADATA
> >>    btrfs: cleanup device states define BTRFS_DEV_STATE_MISSING
> >>    btrfs: cleanup device states define BTRFS_DEV_STATE_REPLACE_TGT
> >>    btrfs: cleanup device states define BTRFS_DEV_STATE_FLUSH_SENT
> > 
> > 1-5 added to next. I had to tweak the whitespace in the conditions.
> 
>   Oops I did run, checkpatch, not too sure how did I still missed it.

Checkpatch will not help, this is a matter of style used in btrfs code.
We should tweak the coding style so it looks consistent and familiar to
us. I read a lot of code so it's quite obvious to me and need to fix it
if it's feasible or ask the submitter.

An example:

@@ -3403,7 +3403,8 @@  static int barrier_all_devices(struct btrfs_fs_info *info)
 			continue;
 		if (!dev->bdev)
 			continue;
-		if (!dev->in_fs_metadata || !dev->writeable)
+		if (!dev->in_fs_metadata ||
+			!test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))
 			continue;
 
 		write_dev_flush(dev);

The condition on the next line should start under the first one like

-		if (!dev->in_fs_metadata || !dev->writeable)
+		if (!dev->in_fs_metadata ||
+		    !test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))
 			continue;

As it makes a bit clear what's the condition and what's the statement.
This can become tricky with condition terms that do not fit on one line
or are tested in ( ) :

@@ -3403,8 +3403,10 @@  static int barrier_all_devices(struct btrfs_fs_info *info)
 			continue;
 		if (!dev->bdev)
 			continue;
-		if (!dev->in_fs_metadata ||
-			!test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))
+		if (!test_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
+						&dev->dev_state) ||
+			!test_bit(BTRFS_DEV_STATE_WRITEABLE,
+						&dev->dev_state))
 			continue;

Became

@@ -3395,7 +3395,7 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
                        continue;
                if (!dev->bdev)
                        continue;
-               if (!dev->in_fs_metadata ||
+               if (!test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &dev->dev_state) ||
                    !test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))
                        continue;

You can notice in other patches, that the || operator ends up on column 81,
which is exactly where checkpatch would complain but I will not, as the
operator is completely hidden. The end result is IMHO better.