[v2,0/5] btrfs: sysfs, cleanups
mbox series

Message ID 1574328814-12263-1-git-send-email-anand.jain@oracle.com
Headers show
Series
  • btrfs: sysfs, cleanups
Related show

Message

Anand Jain Nov. 21, 2019, 9:33 a.m. UTC
v2:
  As it is learnt that we might add sysfs attributes for the
  scanned devices at some point, V2 does not cleanup those which
  might be required to implement it. Last attempt to add is here
  [1] in the mailing list.
   [1] [PATCH] btrfs: Introduce device pool sysfs attributes

  This set also drops the patches which moved the local struct declare
  to the place where it was actually used. Agreed top of the file is
  better.

  Drops the patches which was trying to bring the related functions
  together in sysfs.c. I am not sure why this isn't a nice cleanup,
  as in super.c we don't place exit_btrfs_fs() and init_btrfs_fs()
  apart we place them together the idea was similar in sysfs.c.
  Anyway as there is disagreement I have dropped them from the list.

  Dropped the patch '[PATCH 07/15] btrfs: sysfs, delete code in a
  comment' as its already integrated in misc-next.

  In v2 I have used better naming compared to v1. For example
  btrfs_fs_devices::devices_dir_kobj vs btrfs_fs_devices::devices_kobj
  and btrfs_sysfs_add_device_info() vs btrfs_sysfs_add_devices_attr,
  as I am following a pattern that sysfs directories are inherently
  kobjects, which holds files as attributes. We could drop sysfs prefix
  also because kobj and attr already indicate that they are part of
  sysfs. But people not familiar with sysfs terminology might have to
  wonder a little bit, so didn't make that bold changes.

  And I hope I didn't miss any other changes in v2.

Testing:

 As this is a cleanup patches, it should be made sure not to introduce any
 regression in the sysfs layout. So here is a script which compares with
 the golden md5sum of the sysfs layout. There are two golden md5sum
 because as I am using find command which looks like due to some
 optimization the list of file was in the same order all the time, it was
 in two different orders. If there is any better ways to do this, or tell
 find to not to optimize I will be happy to know and use it.

 (sorry for the hard-coded device paths).
-----------8<----------------
#!/bin/bash

known_uuid="12345678-1234-1234-1234-123456789012"
golden_sum1="7ae7b72ee3e5efc32f2fab152dd0fcd5"
golden_sum2="6cefff853e55a31047c57f5a14ffb636"

cleanup()
{
	umount /btrfs > /dev/null 2>&1
	modprobe -r btrfs
}

test()
{
	cleanup

	mkfs.btrfs -fq -draid1 -mraid1 /dev/sdb /dev/sdc
	uuid=$(btrfs fi show /dev/sdb | grep uuid | awk '{print $4}')

	mount -o compress=lzo,device=/dev/sdc /dev/sdb /btrfs
	xfs_io -f -c "pwrite -S 0xab 0 500M" /btrfs/foo  > /dev/null

	find -O0 /sys/fs/btrfs -type f | sed s/$uuid/$known_uuid/g > /tmp/golden.output
	test_sum=$(md5sum /tmp/golden.output|awk '{print $1}')

	umount /btrfs
	modprobe -r btrfs

	[ "$golden_sum1" == "$test_sum" ] && return
	[ "$golden_sum2" == "$test_sum" ] && return

	echo "Failed: test_sum $test_sum"
	echo "Golden:        1 $golden_sum1"
	echo "Golden:        2 $golden_sum2"
}

test
-----------8<----------------



v1: cover-letter

Mostly cleanups patches.

Patches 1-7 are renames, code moves patches and there are no
functional changes.

Patch 8 drops unused argument in the function btrfs_sysfs_add_fsid().
Patch 9 merges two small functions which is an extension of the other.

Patches 10,11 and 13 removes unnecessary features in the functions, 
originally it was planned to provide sysfs attributes for the scanned
and unmounted devices, as in the un-merged patch in the mailing list [1]
   [1] [PATCH] btrfs: Introduce device pool sysfs attributes

Patch 12 merges functions.

Patches 14,15 are code optimize patches.

Anand Jain (5):
  btrfs: sysfs, rename devices kobject holder to devices_kobj
  btrfs: sysfs, rename device_link add,remove functions
  btrfs: sysfs, btrfs_sysfs_add_fsid() drop unused argument parent
  btrfs: sysfs, rename btrfs_sysfs_add_device()
  btrfs: sysfs, merge btrfs_sysfs_add devices_kobj and fsid

 fs/btrfs/dev-replace.c |  4 ++--
 fs/btrfs/disk-io.c     |  9 +-------
 fs/btrfs/sysfs.c       | 62 ++++++++++++++++++++++++--------------------------
 fs/btrfs/sysfs.h       |  8 +++----
 fs/btrfs/volumes.c     |  8 +++----
 fs/btrfs/volumes.h     |  2 +-
 6 files changed, 41 insertions(+), 52 deletions(-)

Comments

David Sterba Nov. 26, 2019, 4:54 p.m. UTC | #1
On Thu, Nov 21, 2019 at 05:33:29PM +0800, Anand Jain wrote:
> v2:
>   In v2 I have used better naming compared to v1. For example
>   btrfs_fs_devices::devices_dir_kobj vs btrfs_fs_devices::devices_kobj
>   and btrfs_sysfs_add_device_info() vs btrfs_sysfs_add_devices_attr,
>   as I am following a pattern that sysfs directories are inherently
>   kobjects, which holds files as attributes. We could drop sysfs prefix
>   also because kobj and attr already indicate that they are part of
>   sysfs. But people not familiar with sysfs terminology might have to
>   wonder a little bit, so didn't make that bold changes.

I think the _sysfs_ part of the function is a good thing and makes it
clear that it belongs to the sysfs interfacing wrappers.

> Anand Jain (5):
>   btrfs: sysfs, rename devices kobject holder to devices_kobj
>   btrfs: sysfs, rename device_link add,remove functions
>   btrfs: sysfs, btrfs_sysfs_add_fsid() drop unused argument parent
>   btrfs: sysfs, rename btrfs_sysfs_add_device()
>   btrfs: sysfs, merge btrfs_sysfs_add devices_kobj and fsid

1,3,4,5 applied, thanks.