| Message ID | 1385657947-26145-1-git-send-email-dsterba@suse.cz (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Headers | show |
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
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 -- 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
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 > -- 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
> 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. -- 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
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. -- 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
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 -- 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
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 > -- 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
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. -- 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
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 -- 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
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. -- 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
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 -- 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
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. -- 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 --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
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(-)