[PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[David Sterba]

Subvolume deletion does not do a full transaction commit. This can lead
to an unexpected result when the system crashes between deletion and
commit, the subvolume directory will appear again. Add options to request
filesystem sync after each deleted subvolume or after the last one.

If the command with sync option finishes succesfully, the subvolume(s)
deletion status is safely stored on the media.

Userspace approach is more flexible than in-kernel. Related discussions:
http://www.spinics.net/lists/linux-btrfs/msg22088.html
http://www.spinics.net/lists/linux-btrfs/msg27240.html

CC: Alex Lyakas <alex.btrfs@zadarastorage.com>
Signed-off-by: David Sterba <dsterba@suse.cz>
---

The option names may not be ideal, feel free to suggest better.

 cmds-subvolume.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 man/btrfs.8.in   | 23 ++++++++++++++---
 2 files changed, 90 insertions(+), 10 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index c6a5284a02a4..c124c3185252 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -202,24 +202,67 @@ int test_issubvolume(char *path)
 }
 
 static const char * const cmd_subvol_delete_usage[] = {
-	"btrfs subvolume delete <subvolume> [<subvolume>...]",
+	"btrfs subvolume delete [options] <subvolume> [<subvolume>...]",
 	"Delete subvolume(s)",
+	"Delete subvolumes from the filesystem. The corresponding directory",
+	"is removed instantly but the data blocks are removed later.",
+	"The deletion does not involve full commit by default due to",
+	"performance reasons (as a consequence, the subvolume may appear again",
+	"after a crash). Use one of the --sync options to sync the filesystem",
+	"via a btrfs specific ioctl.",
+	"",
+	"-s|--sync-after      call sync at the end of the operation",
+	"-S|--sync-each       sync the filesystem after deleting each subvolume",
 	NULL
 };
 
 static int cmd_subvol_delete(int argc, char **argv)
 {
-	int	res, fd, len, e, cnt = 1, ret = 0;
+	int	res, len, e, ret = 0;
+	int cnt;
+	int fd = -1;
 	struct btrfs_ioctl_vol_args	args;
 	char	*dname, *vname, *cpath;
 	char	*dupdname = NULL;
 	char	*dupvname = NULL;
 	char	*path;
 	DIR	*dirstream = NULL;
+	int sync_mode = 0;
+	struct option long_options[] = {
+		{"sync-after", no_argument, NULL, 's'},	/* sync mode 1 */
+		{"sync-each", no_argument, NULL, 'S'},	/* sync mode 2 */
+		{NULL, 0, NULL, 0}
+	};
 
-	if (argc < 2)
+	optind = 1;
+	while (1) {
+		int c;
+
+		c = getopt_long(argc, argv, "sS", long_options, NULL);
+		if (c < 0)
+			break;
+
+		switch(c) {
+		case 's':
+			sync_mode = 1;
+			break;
+		case 'S':
+			sync_mode = 2;
+			break;
+		default:
+			usage(cmd_subvol_delete_usage);
+		}
+	}
+
+	if (check_argc_min(argc - optind, 1))
 		usage(cmd_subvol_delete_usage);
 
+	printf("Sync filesystem mode: %s\n",
+		!sync_mode ? "none (default)" :
+		sync_mode == 1 ? "at the end" : "after each");
+
+	cnt = optind;
+
 again:
 	path = argv[cnt];
 
@@ -276,8 +319,6 @@ again:
 	res = ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &args);
 	e = errno;
 
-	close_file_or_dir(fd, dirstream);
-
 	if(res < 0 ){
 		fprintf( stderr, "ERROR: cannot delete '%s/%s' - %s\n",
 			dname, vname, strerror(e));
@@ -285,14 +326,38 @@ again:
 		goto out;
 	}
 
+	if (sync_mode == 1) {
+		res = ioctl(fd, BTRFS_IOC_SYNC);
+		if (res < 0) {
+			fprintf(stderr,
+				"ERROR: unable to sync after '%s': %s\n",
+				path, strerror(e));
+			ret = 1;
+		}
+	}
+
 out:
 	free(dupdname);
 	free(dupvname);
 	dupdname = NULL;
 	dupvname = NULL;
 	cnt++;
-	if (cnt < argc)
+	if (cnt < argc) {
+		close_file_or_dir(fd, dirstream);
 		goto again;
+	}
+
+	if (sync_mode == 2 && fd != -1) {
+		res = ioctl(fd, BTRFS_IOC_SYNC);
+		e = errno;
+		if (res < 0) {
+			fprintf(stderr,
+				"ERROR: unable to do final sync: %s\n",
+				strerror(e));
+			ret = 1;
+		}
+	}
+	close_file_or_dir(fd, dirstream);
 
 	return ret;
 }
diff --git a/man/btrfs.8.in b/man/btrfs.8.in
index b6203483296e..2d5c12e08640 100644
--- a/man/btrfs.8.in
+++ b/man/btrfs.8.in
@@ -8,7 +8,7 @@ btrfs \- control a btrfs filesystem
 .SH SYNOPSIS
 \fBbtrfs\fP \fBsubvolume create\fP [-i \fI<qgroupid>\fP] [\fI<dest>\fP/]\fI<name>\fP
 .PP
-\fBbtrfs\fP \fBsubvolume delete\fP \fI<subvolume>\fP [\fI<subvolume>...\fP]
+\fBbtrfs\fP \fBsubvolume delete\fP [\fIoptions\fP] \fI<subvolume>\fP [\fI<subvolume>...\fP]
 .PP
 \fBbtrfs\fP \fBsubvolume list\fP [\fIoptions\fP] [-G [+|-]\fIvalue\fP] [-C [+|-]\fIvalue\fP] [--sort=rootid,gen,ogen,path] \fI<path>\fP
 .PP
@@ -168,9 +168,24 @@ times.
 .RE
 .TP
 
-\fBsubvolume delete\fR\fI <subvolume> \fP[\fI<subvolume>...\fP]\fR
-Delete the subvolume \fI<subvolume>\fR. If \fI<subvolume>\fR is not a
-subvolume, \fBbtrfs\fR returns an error.
+\fBsubvolume delete\fR [\fIoptions\fP] \fI<subvolume>\fP [\fI<subvolume>...\fP]\fR
+Delete the subvolume(s) from the filesystem. If \fI<subvolume>\fR is not a
+subvolume, \fBbtrfs\fR returns an error but continues if there are more arguments
+to process.
+
+The corresponding directory is removed instantly but the data blocks are
+removed later.  The deletion does not involve full commit by default due to
+performance reasons (as a consequence, the subvolume may appear again after a
+crash).  Use one of the --sync options to sync the filesystem via a btrfs
+specific ioctl.
+.RS
+
+\fIOptions\fP
+.IP "\fB-s|--sync-after\fP" 5
+call sync at the end of the operation
+.IP "\fB-S|--sync-each\fP" 5
+sync the filesystem after deleting each subvolume
+.RE
 .TP
 
 \fBsubvolume list\fR [\fIoptions\fP] [-G [+|-]\fIvalue\fP] [-C [+|-]\fIvalue\fP] [--sort=rootid,gen,ogen,path] \fI<path>\fR
-- 
1.8.4.3

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[Roman Mamedov]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

On Thu, 28 Nov 2013 17:59:07 +0100
David Sterba <dsterba@suse.cz> wrote:

> Subvolume deletion does not do a full transaction commit. This can lead
> to an unexpected result when the system crashes between deletion and
> commit, the subvolume directory will appear again. Add options to request
> filesystem sync after each deleted subvolume or after the last one.
> 
> If the command with sync option finishes succesfully, the subvolume(s)
> deletion status is safely stored on the media.

A feature that people pretty often request, is a way to delete a subvolume,
and have some way to "sync", i.e. pause execution of their script until all
the space freed up by the deletion has been recovered and made available (e.g.
for the next round of their backup to run).

Does this patch now make this possible?

Also, did you consider enhancing "btrfs fi sync" to sync the snapshot deletion
(it currently doesn't), rather than adding the "sync" options to deletion
itself? E.g. comparing to traditional tools, people would logically do:

  $ rm /some/dir/some/file
  $ sync

rather than something akin to

  $ rm --sync /some/dir/some/file

-- 
With respect,
Roman

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[Wang Shilong]

On 11/29/2013 03:36 AM, Roman Mamedov wrote:
> On Thu, 28 Nov 2013 17:59:07 +0100
> David Sterba <dsterba@suse.cz> wrote:
>
>> Subvolume deletion does not do a full transaction commit. This can lead
>> to an unexpected result when the system crashes between deletion and
>> commit, the subvolume directory will appear again. Add options to request
>> filesystem sync after each deleted subvolume or after the last one.
>>
>> If the command with sync option finishes succesfully, the subvolume(s)
>> deletion status is safely stored on the media.
> A feature that people pretty often request, is a way to delete a subvolume,
> and have some way to "sync", i.e. pause execution of their script until all
> the space freed up by the deletion has been recovered and made available (e.g.
> for the next round of their backup to run).
>
> Does this patch now make this possible?
My understanding here is:

No, deleting subvolume is 'async'. this patch only gurantee that
subvolume deletion status is on-disk, but subvolume deletion work
still have not finished, it will run in the backgroud, if a crash happens,
the work will continue after rebooting.

>
> Also, did you consider enhancing "btrfs fi sync" to sync the snapshot deletion
> (it currently doesn't), rather than adding the "sync" options to deletion
> itself? E.g. comparing to traditional tools, people would logically do:
>
>    $ rm /some/dir/some/file
>    $ sync
>
> rather than something akin to
>
>    $ rm --sync /some/dir/some/file
>
subvolume deletion is a little different from command 'rm'.
To delete a subvolume, we have to run:
btrfs sub delete  <subv>

If you run 'btrfs file sync' after 'btrfs sub delete' it will have same
effect as david's 'btrfs sub delete --sync'  i think.

Thanks,
Wang

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[David Sterba]

On Fri, Nov 29, 2013 at 10:04:35AM +0800, Wang Shilong wrote:
> >>Subvolume deletion does not do a full transaction commit. This can lead
> >>to an unexpected result when the system crashes between deletion and
> >>commit, the subvolume directory will appear again. Add options to request
> >>filesystem sync after each deleted subvolume or after the last one.
> >>
> >>If the command with sync option finishes succesfully, the subvolume(s)
> >>deletion status is safely stored on the media.
> >A feature that people pretty often request, is a way to delete a subvolume,
> >and have some way to "sync", i.e. pause execution of their script until all
> >the space freed up by the deletion has been recovered and made available (e.g.
> >for the next round of their backup to run).
> >
> >Does this patch now make this possible?
> My understanding here is:
> 
> No, deleting subvolume is 'async'. this patch only gurantee that
> subvolume deletion status is on-disk, but subvolume deletion work
> still have not finished, it will run in the backgroud, if a crash happens,
> the work will continue after rebooting.

That's right, but also shows that there are different notions what the
sync could mean and both make sense and should be implemented.

Waiting for the subvolume until it's completely deleted can be now
implemented by polling the dead root key. The subvolume deletion ioctl
takes only the path but no flags so it cannot be extended that way.

> >Also, did you consider enhancing "btrfs fi sync" to sync the snapshot deletion
> >(it currently doesn't), rather than adding the "sync" options to deletion
> >itself? E.g. comparing to traditional tools, people would logically do:
> >
> >   $ rm /some/dir/some/file
> >   $ sync
> >
> >rather than something akin to
> >
> >   $ rm --sync /some/dir/some/file
> >
> subvolume deletion is a little different from command 'rm'.
> To delete a subvolume, we have to run:
> btrfs sub delete  <subv>
> 
> If you run 'btrfs file sync' after 'btrfs sub delete' it will have same
> effect as david's 'btrfs sub delete --sync'  i think.

Yeah, it's different but extending 'fi sync' to wait for subvolume
cleaning is a valid usecase.

So an enahced interface could look like this:

subvol delete:
--commit-each - run the ioc sync/wait ioctl after each delete ioctl
--commit-after - dtto but sync/wait after all are deleted
--wait-for-cleanup - wait until all given subvols are cleaned

'filesystem sync' exteded to wait for subvol cleanup has following
cases:
- wait for a specific subvolume to be cleaned
- wait for all currently deleted, do not care if more subvols are
  deleted in the meantime
- wait until there are no subvolumes left to clean

Are there more usecases to cover?


david

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[Wang Shilong]

Hi Dave,

On 11/30/2013 01:37 AM, David Sterba wrote:
> On Fri, Nov 29, 2013 at 10:04:35AM +0800, Wang Shilong wrote:
>>>> Subvolume deletion does not do a full transaction commit. This can lead
>>>> to an unexpected result when the system crashes between deletion and
>>>> commit, the subvolume directory will appear again. Add options to request
>>>> filesystem sync after each deleted subvolume or after the last one.
>>>>
>>>> If the command with sync option finishes succesfully, the subvolume(s)
>>>> deletion status is safely stored on the media.
>>> A feature that people pretty often request, is a way to delete a subvolume,
>>> and have some way to "sync", i.e. pause execution of their script until all
>>> the space freed up by the deletion has been recovered and made available (e.g.
>>> for the next round of their backup to run).
>>>
>>> Does this patch now make this possible?
>> My understanding here is:
>>
>> No, deleting subvolume is 'async'. this patch only gurantee that
>> subvolume deletion status is on-disk, but subvolume deletion work
>> still have not finished, it will run in the backgroud, if a crash happens,
>> the work will continue after rebooting.
> That's right, but also shows that there are different notions what the
> sync could mean and both make sense and should be implemented.
>
> Waiting for the subvolume until it's completely deleted can be now
> implemented by polling the dead root key. The subvolume deletion ioctl
> takes only the path but no flags so it cannot be extended that way.
>
>>> Also, did you consider enhancing "btrfs fi sync" to sync the snapshot deletion
>>> (it currently doesn't), rather than adding the "sync" options to deletion
>>> itself? E.g. comparing to traditional tools, people would logically do:
>>>
>>>    $ rm /some/dir/some/file
>>>    $ sync
>>>
>>> rather than something akin to
>>>
>>>    $ rm --sync /some/dir/some/file
>>>
>> subvolume deletion is a little different from command 'rm'.
>> To delete a subvolume, we have to run:
>> btrfs sub delete  <subv>
>>
>> If you run 'btrfs file sync' after 'btrfs sub delete' it will have same
>> effect as david's 'btrfs sub delete --sync'  i think.
> Yeah, it's different but extending 'fi sync' to wait for subvolume
> cleaning is a valid usecase.
>
> So an enahced interface could look like this:
>
> subvol delete:
> --commit-each - run the ioc sync/wait ioctl after each delete ioctl
> --commit-after - dtto but sync/wait after all are deleted
> --wait-for-cleanup - wait until all given subvols are cleaned
>
> 'filesystem sync' exteded to wait for subvol cleanup has following
> cases:
> - wait for a specific subvolume to be cleaned
> - wait for all currently deleted, do not care if more subvols are
>    deleted in the meantime
> - wait until there are no subvolumes left to clean
I think it is unnecessary to add such options for 'filesystem sync'.
we may wait a long time until all subvolume deletion are finished as
async subvolume deletion is implemented in cleaner thread.:-)

Miao also pointed out that only if we are running out of space, waiting
subvolume deletion finished will make sense, our opinion is not to disturb
'filesystem sync' .

What do you think ? :-)

Thanks,
Wang
>
> Are there more usecases to cover?
>
>
> david
> --
> 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
>

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[David Sterba]

On Mon, Dec 02, 2013 at 05:02:49PM +0800, Wang Shilong wrote:
> >So an enahced interface could look like this:
> >
> >subvol delete:
> >--commit-each - run the ioc sync/wait ioctl after each delete ioctl
> >--commit-after - dtto but sync/wait after all are deleted
> >--wait-for-cleanup - wait until all given subvols are cleaned
> >
> >'filesystem sync' exteded to wait for subvol cleanup has following
> >cases:
> >- wait for a specific subvolume to be cleaned
> >- wait for all currently deleted, do not care if more subvols are
> >   deleted in the meantime
> >- wait until there are no subvolumes left to clean
> I think it is unnecessary to add such options for 'filesystem sync'.
> we may wait a long time until all subvolume deletion are finished as
> async subvolume deletion is implemented in cleaner thread.:-)

I mean that 'filesystem sync' will stay as it is now, but will be
enhanced with a few options to further specify what else should be
synced.

The difference is that the wait/sync is not tied to the 'subvol delete'
operation. I start to think that a new 'filesystem' subcommand would be
suitable for that, instead of 'fi sync'.

> Miao also pointed out that only if we are running out of space, waiting
> subvolume deletion finished will make sense, our opinion is not to disturb
> 'filesystem sync' .
> 
> What do you think ? :-)

I agree with that.

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[Chris Mason]

Quoting David Sterba (2013-12-09 18:32:45)
> On Mon, Dec 02, 2013 at 05:02:49PM +0800, Wang Shilong wrote:
> > >So an enahced interface could look like this:
> > >
> > >subvol delete:
> > >--commit-each - run the ioc sync/wait ioctl after each delete ioctl
> > >--commit-after - dtto but sync/wait after all are deleted
> > >--wait-for-cleanup - wait until all given subvols are cleaned
> > >
> > >'filesystem sync' exteded to wait for subvol cleanup has following
> > >cases:
> > >- wait for a specific subvolume to be cleaned

It may be hard to wait for a specific subvolume from btrfs fi sync.
You'd have to know the id, or have an interface that shows a list of ids
currently under deletion (not a bad idea ;)

> > >- wait for all currently deleted, do not care if more subvols are
> > >   deleted in the meantime
> > >- wait until there are no subvolumes left to clean
> > I think it is unnecessary to add such options for 'filesystem sync'.
> > we may wait a long time until all subvolume deletion are finished as
> > async subvolume deletion is implemented in cleaner thread.:-)
> 
> I mean that 'filesystem sync' will stay as it is now, but will be
> enhanced with a few options to further specify what else should be
> synced.

It's more natural to have the waiting in the subvol delete command, but
I'm not against adding a few ways to wait in btrfs fi sync too, as long
as they share the same core implementation.

-chris

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[David Sterba]

On Tue, Dec 10, 2013 at 08:17:07AM -0500, Chris Mason wrote:
> Quoting David Sterba (2013-12-09 18:32:45)
> > On Mon, Dec 02, 2013 at 05:02:49PM +0800, Wang Shilong wrote:
> > > >So an enahced interface could look like this:
> > > >
> > > >subvol delete:
> > > >--commit-each - run the ioc sync/wait ioctl after each delete ioctl
> > > >--commit-after - dtto but sync/wait after all are deleted
> > > >--wait-for-cleanup - wait until all given subvols are cleaned
> > > >
> > > >'filesystem sync' exteded to wait for subvol cleanup has following
> > > >cases:
> > > >- wait for a specific subvolume to be cleaned
> 
> It may be hard to wait for a specific subvolume from btrfs fi sync.
> You'd have to know the id, or have an interface that shows a list of ids
> currently under deletion (not a bad idea ;)

It is there, 'btrfs subvol list -d /path', but I'd rather not let
everybody parse output the output for a simple check.

> > > >- wait for all currently deleted, do not care if more subvols are
> > > >   deleted in the meantime
> > > >- wait until there are no subvolumes left to clean
> > > I think it is unnecessary to add such options for 'filesystem sync'.
> > > we may wait a long time until all subvolume deletion are finished as
> > > async subvolume deletion is implemented in cleaner thread.:-)
> > 
> > I mean that 'filesystem sync' will stay as it is now, but will be
> > enhanced with a few options to further specify what else should be
> > synced.
> 
> It's more natural to have the waiting in the subvol delete command, but
> I'm not against adding a few ways to wait in btrfs fi sync too, as long
> as they share the same core implementation.

Yeah it's natural and I guess it'll be the most frequent type of use.
I've tried to think of the possible uses so we don't miss anything
during design phase.

Knowing the ID is necessary, the subvolume path is lost at deletion
time, but let's say that the ID is available somehow, eg from
application logs.

I have a prototype for that, there's a separate subcommand to do just
the waiting for a given subvolume list id to be cleaned up (but could
be merged to fi sync if desired). It's built around the SEARCH ioctl and
does not need kernel support.

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[Chris Mason]

Quoting David Sterba (2013-12-10 12:36:28)
> On Tue, Dec 10, 2013 at 08:17:07AM -0500, Chris Mason wrote:
> > Quoting David Sterba (2013-12-09 18:32:45)
> > > On Mon, Dec 02, 2013 at 05:02:49PM +0800, Wang Shilong wrote:
> > > > >So an enahced interface could look like this:
> > > > >
> > > > >subvol delete:
> > > > >--commit-each - run the ioc sync/wait ioctl after each delete ioctl
> > > > >--commit-after - dtto but sync/wait after all are deleted
> > > > >--wait-for-cleanup - wait until all given subvols are cleaned
> > > > >
> > > > >'filesystem sync' exteded to wait for subvol cleanup has following
> > > > >cases:
> > > > >- wait for a specific subvolume to be cleaned
> > 
> > It may be hard to wait for a specific subvolume from btrfs fi sync.
> > You'd have to know the id, or have an interface that shows a list of ids
> > currently under deletion (not a bad idea ;)
> 
> It is there, 'btrfs subvol list -d /path', but I'd rather not let
> everybody parse output the output for a simple check.

Aha ;) Yeah, that's what I was thinking.  It's possible to find but hard
enough that nobody would be happy using it.

> 
> > > > >- wait for all currently deleted, do not care if more subvols are
> > > > >   deleted in the meantime
> > > > >- wait until there are no subvolumes left to clean
> > > > I think it is unnecessary to add such options for 'filesystem sync'.
> > > > we may wait a long time until all subvolume deletion are finished as
> > > > async subvolume deletion is implemented in cleaner thread.:-)
> > > 
> > > I mean that 'filesystem sync' will stay as it is now, but will be
> > > enhanced with a few options to further specify what else should be
> > > synced.
> > 
> > It's more natural to have the waiting in the subvol delete command, but
> > I'm not against adding a few ways to wait in btrfs fi sync too, as long
> > as they share the same core implementation.
> 
> Yeah it's natural and I guess it'll be the most frequent type of use.
> I've tried to think of the possible uses so we don't miss anything
> during design phase.
> 
> Knowing the ID is necessary, the subvolume path is lost at deletion
> time, but let's say that the ID is available somehow, eg from
> application logs.
> 
> I have a prototype for that, there's a separate subcommand to do just
> the waiting for a given subvolume list id to be cleaned up (but could
> be merged to fi sync if desired). It's built around the SEARCH ioctl and
> does not need kernel support.

So the part where I think we agree is I don't think the kernel subvol
deletion ioctl should do the waiting.  Exactly how the progs wait is a
different question.

-chris

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[David Sterba]

On Tue, Dec 10, 2013 at 01:24:13PM -0500, Chris Mason wrote:
> > It is there, 'btrfs subvol list -d /path', but I'd rather not let
> > everybody parse output the output for a simple check.
> 
> Aha ;) Yeah, that's what I was thinking.  It's possible to find but hard
> enough that nobody would be happy using it.

Sorry I dont' udnerstand, you mean that the -d option is hard to find?

> > Yeah it's natural and I guess it'll be the most frequent type of use.
> > I've tried to think of the possible uses so we don't miss anything
> > during design phase.
> > 
> > Knowing the ID is necessary, the subvolume path is lost at deletion
> > time, but let's say that the ID is available somehow, eg from
> > application logs.
> > 
> > I have a prototype for that, there's a separate subcommand to do just
> > the waiting for a given subvolume list id to be cleaned up (but could
> > be merged to fi sync if desired). It's built around the SEARCH ioctl and
> > does not need kernel support.
> 
> So the part where I think we agree is I don't think the kernel subvol
> deletion ioctl should do the waiting.

Yes, I've abandoned the kernel approach, for several reasons.

> Exactly how the progs wait is a different question.

BTRFS_IOC_START_SYNC + BTRFS_IOC_WAIT_SYNC

I'll send out v2.

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[Miao Xie]

On 	thu, 28 Nov 2013 17:59:07 +0100, David Sterba wrote:
> Subvolume deletion does not do a full transaction commit. This can lead
> to an unexpected result when the system crashes between deletion and
> commit, the subvolume directory will appear again. Add options to request
> filesystem sync after each deleted subvolume or after the last one.
> 
> If the command with sync option finishes succesfully, the subvolume(s)
> deletion status is safely stored on the media.
> 
> Userspace approach is more flexible than in-kernel. Related discussions:
> http://www.spinics.net/lists/linux-btrfs/msg22088.html
> http://www.spinics.net/lists/linux-btrfs/msg27240.html
> 
> CC: Alex Lyakas <alex.btrfs@zadarastorage.com>
> Signed-off-by: David Sterba <dsterba@suse.cz>
> ---
> 
> The option names may not be ideal, feel free to suggest better.
> 
>  cmds-subvolume.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  man/btrfs.8.in   | 23 ++++++++++++++---
>  2 files changed, 90 insertions(+), 10 deletions(-)
> 
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index c6a5284a02a4..c124c3185252 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -202,24 +202,67 @@ int test_issubvolume(char *path)
>  }
>  
>  static const char * const cmd_subvol_delete_usage[] = {
> -	"btrfs subvolume delete <subvolume> [<subvolume>...]",
> +	"btrfs subvolume delete [options] <subvolume> [<subvolume>...]",
>  	"Delete subvolume(s)",
> +	"Delete subvolumes from the filesystem. The corresponding directory",
> +	"is removed instantly but the data blocks are removed later.",
> +	"The deletion does not involve full commit by default due to",
> +	"performance reasons (as a consequence, the subvolume may appear again",
> +	"after a crash). Use one of the --sync options to sync the filesystem",
> +	"via a btrfs specific ioctl.",
> +	"",
> +	"-s|--sync-after      call sync at the end of the operation",
> +	"-S|--sync-each       sync the filesystem after deleting each subvolume",
>  	NULL
>  };
>  
>  static int cmd_subvol_delete(int argc, char **argv)
>  {
> -	int	res, fd, len, e, cnt = 1, ret = 0;
> +	int	res, len, e, ret = 0;
> +	int cnt;
> +	int fd = -1;
>  	struct btrfs_ioctl_vol_args	args;
>  	char	*dname, *vname, *cpath;
>  	char	*dupdname = NULL;
>  	char	*dupvname = NULL;
>  	char	*path;
>  	DIR	*dirstream = NULL;
> +	int sync_mode = 0;
> +	struct option long_options[] = {
> +		{"sync-after", no_argument, NULL, 's'},	/* sync mode 1 */
> +		{"sync-each", no_argument, NULL, 'S'},	/* sync mode 2 */
> +		{NULL, 0, NULL, 0}
> +	};

Generally, we put it out of the functions.

> -	if (argc < 2)
> +	optind = 1;
> +	while (1) {
> +		int c;
> +
> +		c = getopt_long(argc, argv, "sS", long_options, NULL);
> +		if (c < 0)
> +			break;
> +
> +		switch(c) {
> +		case 's':
> +			sync_mode = 1;
> +			break;
> +		case 'S':
> +			sync_mode = 2;
> +			break;
> +		default:
> +			usage(cmd_subvol_delete_usage);
> +		}
> +	}
> +
> +	if (check_argc_min(argc - optind, 1))
>  		usage(cmd_subvol_delete_usage);
>  
> +	printf("Sync filesystem mode: %s\n",
> +		!sync_mode ? "none (default)" :
> +		sync_mode == 1 ? "at the end" : "after each");
> +
> +	cnt = optind;
> +
>  again:
>  	path = argv[cnt];
>  
> @@ -276,8 +319,6 @@ again:
>  	res = ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &args);
>  	e = errno;
>  
> -	close_file_or_dir(fd, dirstream);
> -
>  	if(res < 0 ){
>  		fprintf( stderr, "ERROR: cannot delete '%s/%s' - %s\n",
>  			dname, vname, strerror(e));
> @@ -285,14 +326,38 @@ again:
>  		goto out;
>  	}
>  
> +	if (sync_mode == 1) {
> +		res = ioctl(fd, BTRFS_IOC_SYNC);

It will flush all the dirty pages, it is unnecessary. I think BTRFS_IOC_{START, WAIT}_SYNC is better.

Thanks
Miao

> +		if (res < 0) {
> +			fprintf(stderr,
> +				"ERROR: unable to sync after '%s': %s\n",
> +				path, strerror(e));
> +			ret = 1;
> +		}
> +	}
> +
>  out:
>  	free(dupdname);
>  	free(dupvname);
>  	dupdname = NULL;
>  	dupvname = NULL;
>  	cnt++;
> -	if (cnt < argc)
> +	if (cnt < argc) {
> +		close_file_or_dir(fd, dirstream);
>  		goto again;
> +	}
> +
> +	if (sync_mode == 2 && fd != -1) {
> +		res = ioctl(fd, BTRFS_IOC_SYNC);
> +		e = errno;
> +		if (res < 0) {
> +			fprintf(stderr,
> +				"ERROR: unable to do final sync: %s\n",
> +				strerror(e));
> +			ret = 1;
> +		}
> +	}
> +	close_file_or_dir(fd, dirstream);
>  
>  	return ret;
>  }
> diff --git a/man/btrfs.8.in b/man/btrfs.8.in
> index b6203483296e..2d5c12e08640 100644
> --- a/man/btrfs.8.in
> +++ b/man/btrfs.8.in
> @@ -8,7 +8,7 @@ btrfs \- control a btrfs filesystem
>  .SH SYNOPSIS
>  \fBbtrfs\fP \fBsubvolume create\fP [-i \fI<qgroupid>\fP] [\fI<dest>\fP/]\fI<name>\fP
>  .PP
> -\fBbtrfs\fP \fBsubvolume delete\fP \fI<subvolume>\fP [\fI<subvolume>...\fP]
> +\fBbtrfs\fP \fBsubvolume delete\fP [\fIoptions\fP] \fI<subvolume>\fP [\fI<subvolume>...\fP]
>  .PP
>  \fBbtrfs\fP \fBsubvolume list\fP [\fIoptions\fP] [-G [+|-]\fIvalue\fP] [-C [+|-]\fIvalue\fP] [--sort=rootid,gen,ogen,path] \fI<path>\fP
>  .PP
> @@ -168,9 +168,24 @@ times.
>  .RE
>  .TP
>  
> -\fBsubvolume delete\fR\fI <subvolume> \fP[\fI<subvolume>...\fP]\fR
> -Delete the subvolume \fI<subvolume>\fR. If \fI<subvolume>\fR is not a
> -subvolume, \fBbtrfs\fR returns an error.
> +\fBsubvolume delete\fR [\fIoptions\fP] \fI<subvolume>\fP [\fI<subvolume>...\fP]\fR
> +Delete the subvolume(s) from the filesystem. If \fI<subvolume>\fR is not a
> +subvolume, \fBbtrfs\fR returns an error but continues if there are more arguments
> +to process.
> +
> +The corresponding directory is removed instantly but the data blocks are
> +removed later.  The deletion does not involve full commit by default due to
> +performance reasons (as a consequence, the subvolume may appear again after a
> +crash).  Use one of the --sync options to sync the filesystem via a btrfs
> +specific ioctl.
> +.RS
> +
> +\fIOptions\fP
> +.IP "\fB-s|--sync-after\fP" 5
> +call sync at the end of the operation
> +.IP "\fB-S|--sync-each\fP" 5
> +sync the filesystem after deleting each subvolume
> +.RE
>  .TP
>  
>  \fBsubvolume list\fR [\fIoptions\fP] [-G [+|-]\fIvalue\fP] [-C [+|-]\fIvalue\fP] [--sort=rootid,gen,ogen,path] \fI<path>\fR
> 

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[David Sterba]

On Fri, Nov 29, 2013 at 01:13:43PM +0800, Miao Xie wrote:
> >  static int cmd_subvol_delete(int argc, char **argv)
> >  {
> > -	int	res, fd, len, e, cnt = 1, ret = 0;
> > +	int	res, len, e, ret = 0;
> > +	int cnt;
> > +	int fd = -1;
> >  	struct btrfs_ioctl_vol_args	args;
> >  	char	*dname, *vname, *cpath;
> >  	char	*dupdname = NULL;
> >  	char	*dupvname = NULL;
> >  	char	*path;
> >  	DIR	*dirstream = NULL;
> > +	int sync_mode = 0;
> > +	struct option long_options[] = {
> > +		{"sync-after", no_argument, NULL, 's'},	/* sync mode 1 */
> > +		{"sync-each", no_argument, NULL, 'S'},	/* sync mode 2 */
> > +		{NULL, 0, NULL, 0}
> > +	};
> 
> Generally, we put it out of the functions.

I see that there are several long option definitions inside the function
that uses them, one example is in the same file in cmds_subvol_list, and
I've followed that pattern.

I think it makes a bit more sense to keep the options local to the
function, definition and usage are close together. The does not have
more than one user, although this is possible but unlikely among btrfs
commands.

> > @@ -285,14 +326,38 @@ again:
> >  		goto out;
> >  	}
> >  
> > +	if (sync_mode == 1) {
> > +		res = ioctl(fd, BTRFS_IOC_SYNC);
> 
> It will flush all the dirty pages, it is unnecessary. I think
> BTRFS_IOC_{START, WAIT}_SYNC is better.

I agree and this is what I actually wanted.

Re: [PATCH] btrfs-progs: add options to sync filesystem after subvol delete

[Anand Jain]

> If the command with sync option finishes succesfully, the subvolume(s)
> deletion status is safely stored on the media.


> +	if (sync_mode == 1) {
> +		res = ioctl(fd, BTRFS_IOC_SYNC);
> +		if (res < 0) {
> +			fprintf(stderr,
> +				"ERROR: unable to sync after '%s': %s\n",
> +				path, strerror(e));
> +			ret = 1;
> +		}
> +	}
> +

generalized sync isn't too viable option in all production environment
as it would disturb the steady state performance in some environment.