diff mbox

[V4,2/2] btrfs-progs: Introduce device delete by devid

Message ID 1457518226-601-1-git-send-email-anand.jain@oracle.com (mailing list archive)
State Superseded
Headers show

Commit Message

Anand Jain March 9, 2016, 10:10 a.m. UTC
From: Anand Jain <Anand.Jain@oracle.com>

This patch introduces new option <devid> for the command

  btrfs device delete <device_path|devid>[..]  <mnt>

In a user reported issue on a 3-disk-RAID1, one disk failed with its SB
unreadable. Now with this patch user will have a choice to delete the
device using devid.

The other method we could do, is to match the input device_path to the
available device_paths with in the kernel. But that won't work in all the
cases, like what if user provided mapper path when the path within the
kernel is a non-mapper path.

This patch depends on the below kernel patch for the new feature to work,
however it will fail-back to the old interface for the kernel without the
patch

  Btrfs: Introduce device delete by devid

Signed-off-by: Anand Jain <anand.jain@oracle.com>
[ coding style fixes ]
Signed-off-by: David Sterba <dsterba@suse.com>
---
v4: a. For future benefit we should check for EOPNOTSUPP as well.
    b. Update the changes to be inline with kernel that is
       BTRFS_DEVICE_SPEC_BY_ID and BTRFS_VOL_ARG_V2_FLAGS_SUPPORTED
       naming changes. and
    (c. As I didn't see progs patch matching with the kernel naming
       changes in the ML.)

v3: enahnced btrfs_ioctl_vol_args_v2 to accept devid instead of
    creating a new structure. Thanks to David.
    Changed subject from
      btrfs-progs: device delete to accept devid

v2: update the missed Documentation for delete (not just remove) as well.
    Thanks to Goffredo.
 Documentation/btrfs-device.asciidoc |  4 +--
 cmds-device.c                       | 52 ++++++++++++++++++++++++++++++-------
 ioctl.h                             | 15 ++++++++++-
 3 files changed, 58 insertions(+), 13 deletions(-)

Comments

David Sterba March 9, 2016, 4:55 p.m. UTC | #1
On Wed, Mar 09, 2016 at 06:10:26PM +0800, Anand Jain wrote:
> From: Anand Jain <Anand.Jain@oracle.com>
> 
> This patch introduces new option <devid> for the command
> 
>   btrfs device delete <device_path|devid>[..]  <mnt>
> 
> In a user reported issue on a 3-disk-RAID1, one disk failed with its SB
> unreadable. Now with this patch user will have a choice to delete the
> device using devid.
> 
> The other method we could do, is to match the input device_path to the
> available device_paths with in the kernel. But that won't work in all the
> cases, like what if user provided mapper path when the path within the
> kernel is a non-mapper path.
> 
> This patch depends on the below kernel patch for the new feature to work,
> however it will fail-back to the old interface for the kernel without the
> patch
> 
>   Btrfs: Introduce device delete by devid
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> [ coding style fixes ]
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> v4: a. For future benefit we should check for EOPNOTSUPP as well.
>     b. Update the changes to be inline with kernel that is
>        BTRFS_DEVICE_SPEC_BY_ID and BTRFS_VOL_ARG_V2_FLAGS_SUPPORTED
>        naming changes. and
>     (c. As I didn't see progs patch matching with the kernel naming
>        changes in the ML.)

I did these changes and updated the patch in devel, mentioned in the
changelog as

    [ refresh on top of current code, sync with latest kernel patches, other
      minor changes ]

Please send any followup changes on top of the current devel patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anand Jain March 10, 2016, 3:09 a.m. UTC | #2
> Please send any followup changes on top of the current devel patch.

  I kind of missed this point at the wiki.
    --
    The git repositories on kernel.org are not used for development
    or integration branches.
    --
  Thanks for the update on how things work at your end, which helps
  to keep loss of productive at low.

  Suggest that we need to update similar for the kernel
  section as well. I see some of the inactive branches
  being mentioned there. And its bit confusing as of now.

-ASJ
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba March 10, 2016, 4:40 p.m. UTC | #3
On Thu, Mar 10, 2016 at 11:09:29AM +0800, Anand Jain wrote:
> 
> > Please send any followup changes on top of the current devel patch.
> 
>   I kind of missed this point at the wiki.
>     --
>     The git repositories on kernel.org are not used for development
>     or integration branches.
>     --
>   Thanks for the update on how things work at your end, which helps
>   to keep loss of productive at low.

The development branch workflow in progs has been used for months, I
don't know why that would be news for you.

>   Suggest that we need to update similar for the kernel
>   section as well. I see some of the inactive branches
>   being mentioned there. And its bit confusing as of now.

That's probably the section about btrfs-next tree that is not active for
a long time. Apparently nobody cared enough to fix it which just
underlines lack of active wiki editors.

I don't understand why you bring it up now, you're a long-term
contributor so you're supposed to know how the patches get merged.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yauhen Kharuzhy March 14, 2016, 5:48 p.m. UTC | #4
On Thu, Mar 10, 2016 at 05:40:56PM +0100, David Sterba wrote:
> On Thu, Mar 10, 2016 at 11:09:29AM +0800, Anand Jain wrote:
> > 
> > > Please send any followup changes on top of the current devel patch.
> > 
> >   I kind of missed this point at the wiki.
> >     --
> >     The git repositories on kernel.org are not used for development
> >     or integration branches.
> >     --
> >   Thanks for the update on how things work at your end, which helps
> >   to keep loss of productive at low.
> 
> The development branch workflow in progs has been used for months, I
> don't know why that would be news for you.
> 
> >   Suggest that we need to update similar for the kernel
> >   section as well. I see some of the inactive branches
> >   being mentioned there. And its bit confusing as of now.
> 
> That's probably the section about btrfs-next tree that is not active for
> a long time. Apparently nobody cared enough to fix it which just
> underlines lack of active wiki editors.
> 
> I don't understand why you bring it up now, you're a long-term
> contributor so you're supposed to know how the patches get merged.

This is actual question for me too. Now I track Chris Mason's git tree
for latest integrated changes but if any other active git repositories
exist, please say. We are like to use btrfs actively in future (and use
it now) in our customer's products, so I intend to be involved in
development.


And last question about development process: can I hope that btrfs
maintainers will use the patchwork.kernel.org for patches tracking?
It's sometimes very difficult to track status of patches sent to
mailing list. For instance, I am trying to get Anand's patches for
'global hot spare' functionality working, they depends from two
another patchsets which were re-sent few times with changes and
introduced new bugs etc...

Btw, Anand, if you have public git repo with 'global hotspare' patches,
could you please send it's location also?
David Sterba March 14, 2016, 6:36 p.m. UTC | #5
On Mon, Mar 14, 2016 at 08:48:44PM +0300, Yauhen Kharuzhy wrote:
> On Thu, Mar 10, 2016 at 05:40:56PM +0100, David Sterba wrote:
> > On Thu, Mar 10, 2016 at 11:09:29AM +0800, Anand Jain wrote:
> > > 
> > > > Please send any followup changes on top of the current devel patch.
> > > 
> > >   I kind of missed this point at the wiki.
> > >     --
> > >     The git repositories on kernel.org are not used for development
> > >     or integration branches.
> > >     --
> > >   Thanks for the update on how things work at your end, which helps
> > >   to keep loss of productive at low.
> > 
> > The development branch workflow in progs has been used for months, I
> > don't know why that would be news for you.
> > 
> > >   Suggest that we need to update similar for the kernel
> > >   section as well. I see some of the inactive branches
> > >   being mentioned there. And its bit confusing as of now.
> > 
> > That's probably the section about btrfs-next tree that is not active for
> > a long time. Apparently nobody cared enough to fix it which just
> > underlines lack of active wiki editors.
> > 
> > I don't understand why you bring it up now, you're a long-term
> > contributor so you're supposed to know how the patches get merged.
> 
> This is actual question for me too. Now I track Chris Mason's git tree
> for latest integrated changes but if any other active git repositories
> exist, please say. We are like to use btrfs actively in future (and use
> it now) in our customer's products, so I intend to be involved in
> development.

Partially covered at

https://btrfs.wiki.kernel.org/index.php/Btrfs_source_repositories

depends if you want btrfs-progs or kernel.

> And last question about development process: can I hope that btrfs
> maintainers will use the patchwork.kernel.org for patches tracking?

We've tried. I had to script around patchwork heavily to make it usable
but it's still not my priority to keep the patchwork status up to date.
I do run some scripts after releases to mark the done and actually
merged, but this may miss patches that got renamed subject.

> It's sometimes very difficult to track status of patches sent to
> mailing list. For instance, I am trying to get Anand's patches for
> 'global hot spare' functionality working, they depends from two
> another patchsets which were re-sent few times with changes and
> introduced new bugs etc...

Well, the number of submitted patchsets is larger than the review
bandwidth required to get them merged. From my personal standpoint, I
monitor patches from areas close to me or features I'd like to get
merged. And I don't manage to cover all of them, unlikely in the
timeframe they're submitted. Most patchsets have impact on various
internals and introduce/modify the interfaces, such things need review
from people who understand the code or the problematics.

Sending a patchset repeatedly helps, it's kind of a reminder for us and
that the author cares about it. Separating patches to small reviewable
pieces is basically considered a good practice, sending preparatory
patchsets that do cleanups or introduce stuff that would be used later
helps to focus on the core changes when they come.

It's considered ok in btrfs community to send emails or ping on IRC and
ask people privately about the plans or status.

HTH.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/btrfs-device.asciidoc b/Documentation/btrfs-device.asciidoc
index 2827598a37f5..bd878f4c33e1 100644
--- a/Documentation/btrfs-device.asciidoc
+++ b/Documentation/btrfs-device.asciidoc
@@ -74,10 +74,10 @@  do not perform discard by default
 -f|--force::::
 force overwrite of existing filesystem on the given disk(s)
 
-*remove* <dev> [<dev>...] <path>::
+*remove* <dev>|<devid> [<dev>|<devid>...] <path>::
 Remove device(s) from a filesystem identified by <path>.
 
-*delete* <dev> [<dev>...] <path>::
+*delete* <dev>|<devid> [<dev>|<devid>...] <path>::
 Alias of remove kept for backwards compatability
 
 *ready* <device>::
diff --git a/cmds-device.c b/cmds-device.c
index 50c1c5dfb394..fcb2735e18ce 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -158,19 +158,45 @@  static int _cmd_device_remove(int argc, char **argv,
 	for(i=1 ; i < argc - 1; i++ ){
 		struct	btrfs_ioctl_vol_args arg;
 		int	res;
-
-		if (is_block_device(argv[i]) != 1 && strcmp(argv[i], "missing")) {
-			error("not a block device: %s", argv[i]);
+		struct btrfs_ioctl_vol_args_v2 argv2 = {0};
+		int is_devid = 0;
+
+		if (string_is_numerical(argv[i])) {
+			argv2.devid = arg_strtou64(argv[i]);
+			argv2.flags = BTRFS_DEVICE_SPEC_BY_ID;
+			is_devid = 1;
+		} else if (is_block_device(argv[i]) == 1
+				|| strcmp(argv[i], "missing") == 0) {
+			strncpy_null(argv2.name, argv[i]);
+		} else {
+			fprintf(stderr,
+				"ERROR: %s is not a block device or devid\n",
+				argv[i]);
 			ret++;
 			continue;
 		}
-		memset(&arg, 0, sizeof(arg));
-		strncpy_null(arg.name, argv[i]);
 		/*
 		 * Positive values are from BTRFS_ERROR_DEV_*,
 		 * otherwise it's a generic error, one of errnos
 		 */
-		res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg);
+		res = ioctl(fdmnt, BTRFS_IOC_RM_DEV_V2, &argv2);
+
+		/*
+		 * if BTRFS_IOC_RM_DEV_V2 is not supported we get ENOTTY and if
+		 * argv2.flags includes a flag which kernel don't understand then
+		 * we shall get EOPNOTSUPP
+		 */
+		if (res && (errno == EOPNOTSUPP || errno == ENOTTY)) {
+			if (is_devid) {
+				fprintf(stderr,
+				"ERROR: kernel does not support delete by devid\n");
+				ret = -EOPNOTSUPP;
+				continue;
+			}
+			memset(&arg, 0, sizeof(arg));
+			strncpy_null(arg.name, argv[i]);
+			res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg);
+		}
 		if (res) {
 			const char *msg;
 
@@ -178,8 +204,14 @@  static int _cmd_device_remove(int argc, char **argv,
 				msg = btrfs_err_str(res);
 			else
 				msg = strerror(errno);
-			error("error removing device '%s': %s",
-				argv[i], msg);
+
+			if (is_devid) {
+				error("error removing devid '%llu': %s",
+					(unsigned long long)argv2.devid, msg);
+			} else {
+				error("error removing device '%s': %s",
+								argv[i], msg);
+			}
 			ret++;
 		}
 	}
@@ -189,7 +221,7 @@  static int _cmd_device_remove(int argc, char **argv,
 }
 
 static const char * const cmd_device_remove_usage[] = {
-	"btrfs device remove <device> [<device>...] <path>",
+	"btrfs device remove <device>|<devid> [<device>|<devid>...] <path>",
 	"Remove a device from a filesystem",
 	NULL
 };
@@ -200,7 +232,7 @@  static int cmd_device_remove(int argc, char **argv)
 }
 
 static const char * const cmd_device_delete_usage[] = {
-	"btrfs device delete <device> [<device>...] <path>",
+	"btrfs device delete <device>|<devid> [<device>|<devid>...] <path>",
 	"Remove a device from a filesystem",
 	NULL
 };
diff --git a/ioctl.h b/ioctl.h
index 771da23160f3..9912a2bc98c7 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -45,6 +45,14 @@  struct btrfs_ioctl_vol_args {
 #define BTRFS_SUBVOL_CREATE_ASYNC	(1ULL << 0)
 #define BTRFS_SUBVOL_RDONLY		(1ULL << 1)
 #define BTRFS_SUBVOL_QGROUP_INHERIT	(1ULL << 2)
+#define BTRFS_DEVICE_SPEC_BY_ID		(1ULL << 3)
+
+#define BTRFS_VOL_ARG_V2_FLAGS_SUPPORTED		\
+			(BTRFS_SUBVOL_CREATE_ASYNC |	\
+			BTRFS_SUBVOL_RDONLY |		\
+			BTRFS_SUBVOL_QGROUP_INHERIT |	\
+			BTRFS_DEVICE_SPEC_BY_ID)
+
 #define BTRFS_FSID_SIZE 16
 #define BTRFS_UUID_SIZE 16
 
@@ -84,7 +92,10 @@  struct btrfs_ioctl_vol_args_v2 {
 		};
 		__u64 unused[4];
 	};
-	char name[BTRFS_SUBVOL_NAME_MAX + 1];
+	union {
+		char name[BTRFS_SUBVOL_NAME_MAX + 1];
+		u64 devid;
+	};
 };
 
 /*
@@ -709,6 +720,8 @@  static inline char *btrfs_err_str(enum btrfs_err_code err_code)
                                   struct btrfs_ioctl_feature_flags[2])
 #define BTRFS_IOC_GET_SUPPORTED_FEATURES _IOR(BTRFS_IOCTL_MAGIC, 57, \
                                   struct btrfs_ioctl_feature_flags[3])
+#define BTRFS_IOC_RM_DEV_V2 _IOW(BTRFS_IOCTL_MAGIC, 58, \
+				   struct btrfs_ioctl_vol_args_v2)
 #ifdef __cplusplus
 }
 #endif