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