| Message ID | 20140522135653.6110.79891.stgit@hds.com |
|---|---|
| State | New |
| Headers | show |
Quoting Tomoki Sekiyama (2014-05-22 08:56:53) > When an array of mount point paths is specified as 'mountpoints' argument > of guest-fsfreeze-freeze, qemu-ga with this patch will only freeze the file > systems mounted on specified paths in Linux. > This would be useful when the host wants to create partial disk snapshots. Since this isn't really applicable for win32, and it's hard to discover optional params via guest-info without some extensive changes to how we handle capabilities negotiation, I think it makes more sense to introduce a new command for this, something like guest-fsfreeze-freeze-filesystems, which we can easily discover and properly mark as unsupported on win32. Other than that looks good. > > Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> > --- > qga/commands-posix.c | 17 ++++++++++++++++- > qga/commands-win32.c | 3 ++- > qga/qapi-schema.json | 4 ++++ > 3 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 34ddba0..771f00c 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -714,9 +714,11 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp) > * Walk list of mounted file systems in the guest, and freeze the ones which > * are real local file systems. > */ > -int64_t qmp_guest_fsfreeze_freeze(Error **errp) > +int64_t qmp_guest_fsfreeze_freeze(bool has_mountpoints, strList *mountpoints, > + Error **errp) > { > int ret = 0, i = 0; > + strList *list; > FsMountList mounts; > struct FsMount *mount; > Error *local_err = NULL; > @@ -741,6 +743,19 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp) > ga_set_frozen(ga_state); > > QTAILQ_FOREACH_REVERSE(mount, &mounts, FsMountList, next) { > + /* To issue fsfreeze in the reverse order of mounts, check if the > + * mount is listed in the list here */ > + if (has_mountpoints) { > + for (list = mountpoints; list; list = list->next) { > + if (strcmp(list->value, mount->dirname) == 0) { > + break; > + } > + } > + if (!list) { > + continue; > + } > + } > + > fd = qemu_open(mount->dirname, O_RDONLY); > if (fd == -1) { > error_setg_errno(errp, errno, "failed to open %s", mount->dirname); > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index d793dd0..0c6296d 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -171,7 +171,8 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp) > * Freeze local file systems using Volume Shadow-copy Service. > * The frozen state is limited for up to 10 seconds by VSS. > */ > -int64_t qmp_guest_fsfreeze_freeze(Error **errp) > +int64_t qmp_guest_fsfreeze_freeze(bool has_mountpoints, strList *mountpoints, > + Error **errp) > { > int i; > Error *local_err = NULL; > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index a8cdcb3..31c0dc8 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -378,12 +378,16 @@ > # > # Sync and freeze all freezable, local guest filesystems > # > +# @mountpoints: #optional an array of mountpoints of filesystems to be frozen. > +# If omitted, every mounted filesystem is frozen. (Since: 2.1) > +# > # Returns: Number of file systems currently frozen. On error, all filesystems > # will be thawed. > # > # Since: 0.15.0 > ## > { 'command': 'guest-fsfreeze-freeze', > + 'data': { '*mountpoints': ['str'] }, > 'returns': 'int' } > > ##
On 06/03/2014 03:21 PM, Michael Roth wrote: > Quoting Tomoki Sekiyama (2014-05-22 08:56:53) >> When an array of mount point paths is specified as 'mountpoints' argument >> of guest-fsfreeze-freeze, qemu-ga with this patch will only freeze the file >> systems mounted on specified paths in Linux. >> This would be useful when the host wants to create partial disk snapshots. > > Since this isn't really applicable for win32, and it's hard to discover > optional params via guest-info without some extensive changes to how we handle > capabilities negotiation, I think it makes more sense to introduce a new > command for this, something like guest-fsfreeze-freeze-filesystems, which we > can easily discover and properly mark as unsupported on win32. Bikeshedding on the proposed name: given that 'fs' is an abbreviation of 'filesystem', "fsfreeze-freeze-filesystems" sounds rather redundant. I would suggest guest-fsfreeze-list as a shorter name that conveys the intent, without quite as much repetition. > > Other than that looks good. >
Quoting Eric Blake (2014-06-03 16:38:35) > On 06/03/2014 03:21 PM, Michael Roth wrote: > > Quoting Tomoki Sekiyama (2014-05-22 08:56:53) > >> When an array of mount point paths is specified as 'mountpoints' argument > >> of guest-fsfreeze-freeze, qemu-ga with this patch will only freeze the file > >> systems mounted on specified paths in Linux. > >> This would be useful when the host wants to create partial disk snapshots. > > > > Since this isn't really applicable for win32, and it's hard to discover > > optional params via guest-info without some extensive changes to how we handle > > capabilities negotiation, I think it makes more sense to introduce a new > > command for this, something like guest-fsfreeze-freeze-filesystems, which we > > can easily discover and properly mark as unsupported on win32. > > Bikeshedding on the proposed name: given that 'fs' is an abbreviation of > 'filesystem', "fsfreeze-freeze-filesystems" sounds rather redundant. I > would suggest guest-fsfreeze-list as a shorter name that conveys the > intent, without quite as much repetition. Somewhat agree, though I think we should retain the guest-<command_group>-<verb> structure and at least go with guest-fsfreeze-freeze-list. I do think that is easy to confuse with 'get me a list of frozen mounts', but probably nothing a little documentation shouldn't clarify. I'll throw guest-fsfreeze-freeze-mountpoints out there, but don't have a strong preference either way. > > > > > Other than that looks good. > > > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org
On 06/03/2014 03:58 PM, Michael Roth wrote: >> Bikeshedding on the proposed name: given that 'fs' is an abbreviation of >> 'filesystem', "fsfreeze-freeze-filesystems" sounds rather redundant. I >> would suggest guest-fsfreeze-list as a shorter name that conveys the >> intent, without quite as much repetition. > > Somewhat agree, though I think we should retain the guest-<command_group>-<verb> > structure and at least go with guest-fsfreeze-freeze-list. guest- prefix is uncontroversial command_group is 'fsfreeze'. Currently in that group are the verbs 'status', 'freeze', and 'thaw'; and my proposal is to add 'list' (not 'freeze-list') as the new verb (just as we don't have 'freeze-status' as a verb). Oh, and adding this command to freeze by list may require a change to the existing guest-fsfreeze-status command to report a third enum value of 'partial' (neither fully 'thawed' nor fully 'frozen').
On 06/03/2014 04:06 PM, Eric Blake wrote: > On 06/03/2014 03:58 PM, Michael Roth wrote: > >>> Bikeshedding on the proposed name: given that 'fs' is an abbreviation of >>> 'filesystem', "fsfreeze-freeze-filesystems" sounds rather redundant. I >>> would suggest guest-fsfreeze-list as a shorter name that conveys the >>> intent, without quite as much repetition. >> >> Somewhat agree, though I think we should retain the guest-<command_group>-<verb> >> structure and at least go with guest-fsfreeze-freeze-list. > > guest- prefix is uncontroversial > command_group is 'fsfreeze'. > Currently in that group are the verbs 'status', 'freeze', and 'thaw'; > and my proposal is to add 'list' (not 'freeze-list') as the new verb > (just as we don't have 'freeze-status' as a verb). Uggh, now that I reread the thread... 'freeze-list' is indeed a better verb than 'list' - we aren't listing the mountpoints (that is patch 2/2 with guest-fs-get-info), but freezing a list of mountpoints (the existing 'freeze' action is taken, so we are doing a new action based on freeze but with a longer name). Okay, I can live with 'guest-fsfreeze-freeze-list'. Do we need a guest-fsfreeze-thaw-list counterpart, or is it sufficient to always thaw all systems without worrying about listing them?
On 6/3/14 18:10 , "Eric Blake" <eblake@redhat.com> wrote: >On 06/03/2014 04:06 PM, Eric Blake wrote: >> On 06/03/2014 03:58 PM, Michael Roth wrote: >> >>>> Bikeshedding on the proposed name: given that 'fs' is an abbreviation >>>>of >>>> 'filesystem', "fsfreeze-freeze-filesystems" sounds rather redundant. >>>>I >>>> would suggest guest-fsfreeze-list as a shorter name that conveys the >>>> intent, without quite as much repetition. >>> >>> Somewhat agree, though I think we should retain the >>>guest-<command_group>-<verb> >>> structure and at least go with guest-fsfreeze-freeze-list. >> >> guest- prefix is uncontroversial >> command_group is 'fsfreeze'. >> Currently in that group are the verbs 'status', 'freeze', and 'thaw'; >> and my proposal is to add 'list' (not 'freeze-list') as the new verb >> (just as we don't have 'freeze-status' as a verb). > >Uggh, now that I reread the thread... > >'freeze-list' is indeed a better verb than 'list' - we aren't listing >the mountpoints (that is patch 2/2 with guest-fs-get-info), but freezing >a list of mountpoints (the existing 'freeze' action is taken, so we are >doing a new action based on freeze but with a longer name). > >Okay, I can live with 'guest-fsfreeze-freeze-list'. I'm okay with 'guest-fsfreeze-freeze-list' too. >Do we need a guest-fsfreeze-thaw-list counterpart, or is it sufficient >to always thaw all systems without worrying about listing them? I think current guest-fsfreeze-thaw is sufficient; don't want to risk leaving some filesystems unfrozen that may cause deadlocks.. Thanks, Tomoki Sekiyama
Quoting Eric Blake (2014-06-03 17:06:22) > On 06/03/2014 03:58 PM, Michael Roth wrote: > > >> Bikeshedding on the proposed name: given that 'fs' is an abbreviation of > >> 'filesystem', "fsfreeze-freeze-filesystems" sounds rather redundant. I > >> would suggest guest-fsfreeze-list as a shorter name that conveys the > >> intent, without quite as much repetition. > > > > Somewhat agree, though I think we should retain the guest-<command_group>-<verb> > > structure and at least go with guest-fsfreeze-freeze-list. > > guest- prefix is uncontroversial > command_group is 'fsfreeze'. > Currently in that group are the verbs 'status', 'freeze', and 'thaw'; > and my proposal is to add 'list' (not 'freeze-list') as the new verb > (just as we don't have 'freeze-status' as a verb). > > Oh, and adding this command to freeze by list may require a change to > the existing guest-fsfreeze-status command to report a third enum value > of 'partial' (neither fully 'thawed' nor fully 'frozen'). Since multiple disk snapshots can be done concurrently, and it's possible management might initiate snapshotting for another disk while a prior snapshot is still being done, being able to freeze a separate subset of filesystems while others are still in a frozen state (and, unfreeze a subset of filesystem while leaving others are left frozen) does seem desirable. OTOH (and I know this may be relying too much on implementation details), technically you only need to flush/freeze the filesystems long enough for QEMU to pivot over to another overlay image (correct me if I'm wrong on how libvirt actually handles this), at which point you should have an immutable, data-consistent backing chain that can be backed up in the background. So, at least for the common use-case, the extra book-keeping doesn't seem very beneficial. In any case, since the current implementation doesn't allow for freezing additional filesystems when others are still frozen, I think that sort of handling could be done as a follow-up. And until that sort of behavior is supported I don't think a 'partial' state really has a useful meaning for users. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org
diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 34ddba0..771f00c 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -714,9 +714,11 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp) * Walk list of mounted file systems in the guest, and freeze the ones which * are real local file systems. */ -int64_t qmp_guest_fsfreeze_freeze(Error **errp) +int64_t qmp_guest_fsfreeze_freeze(bool has_mountpoints, strList *mountpoints, + Error **errp) { int ret = 0, i = 0; + strList *list; FsMountList mounts; struct FsMount *mount; Error *local_err = NULL; @@ -741,6 +743,19 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp) ga_set_frozen(ga_state); QTAILQ_FOREACH_REVERSE(mount, &mounts, FsMountList, next) { + /* To issue fsfreeze in the reverse order of mounts, check if the + * mount is listed in the list here */ + if (has_mountpoints) { + for (list = mountpoints; list; list = list->next) { + if (strcmp(list->value, mount->dirname) == 0) { + break; + } + } + if (!list) { + continue; + } + } + fd = qemu_open(mount->dirname, O_RDONLY); if (fd == -1) { error_setg_errno(errp, errno, "failed to open %s", mount->dirname); diff --git a/qga/commands-win32.c b/qga/commands-win32.c index d793dd0..0c6296d 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -171,7 +171,8 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp) * Freeze local file systems using Volume Shadow-copy Service. * The frozen state is limited for up to 10 seconds by VSS. */ -int64_t qmp_guest_fsfreeze_freeze(Error **errp) +int64_t qmp_guest_fsfreeze_freeze(bool has_mountpoints, strList *mountpoints, + Error **errp) { int i; Error *local_err = NULL; diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index a8cdcb3..31c0dc8 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -378,12 +378,16 @@ # # Sync and freeze all freezable, local guest filesystems # +# @mountpoints: #optional an array of mountpoints of filesystems to be frozen. +# If omitted, every mounted filesystem is frozen. (Since: 2.1) +# # Returns: Number of file systems currently frozen. On error, all filesystems # will be thawed. # # Since: 0.15.0 ## { 'command': 'guest-fsfreeze-freeze', + 'data': { '*mountpoints': ['str'] }, 'returns': 'int' } ##
When an array of mount point paths is specified as 'mountpoints' argument of guest-fsfreeze-freeze, qemu-ga with this patch will only freeze the file systems mounted on specified paths in Linux. This would be useful when the host wants to create partial disk snapshots. Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- qga/commands-posix.c | 17 ++++++++++++++++- qga/commands-win32.c | 3 ++- qga/qapi-schema.json | 4 ++++ 3 files changed, 22 insertions(+), 2 deletions(-)