| Message ID | 20240311174044.67094-1-alexander.ivanov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Use LVM tools for LV block device truncation | expand |
On Mon, Mar 11, 2024 at 06:40:44PM +0100, Alexander Ivanov wrote: > If a block device is an LVM logical volume we can resize it using > standard LVM tools. > > In raw_co_truncate() check if the block device is a LV using lvdisplay > and resize it executing lvresize. > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> > --- > block/file-posix.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 35684f7e21..cf8a04e6f7 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -2670,6 +2670,33 @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset, > if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) { > int64_t cur_length = raw_getlength(bs); > > + /* > + * Check if the device is an LVM logical volume and try to resize > + * it using LVM tools. > + */ > + if (S_ISBLK(st.st_mode) && offset > 0) { > + char cmd[PATH_MAX + 32]; > + > + snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null", > + bs->filename); PATH_MAX + snprint is a bad practice - g_strdup_printf() is recommended for dynamic allocation, along with g_autofree for release. > + ret = system(cmd); IMHO using 'system' for spawning processes is dubious in any non-trivial program. Historically at least it does not have well defined behaviour wrt signal handling in the child, before execve is called. ie potentially a signal handler registered by QEMU in the parent could run in the child having ill-effects. 'system' also executes via the shell which opens up many risks with unsanitized files path being substituted into the command line. > + if (ret != 0) { > + error_setg(errp, "lvdisplay returned %d error for '%s'", > + ret, bs->filename); > + return ret; > + } Calling 'lvdisplay' doesn't seem to be needed. Surely 'lvresize' is going to report errors if it isn't an LVM device. If we want to detect an LVM device though, couldn't we lookup 'device-mapper' in /proc/devices and then major the device major node number. > + > + snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s > /dev/null", > + offset, bs->filename); > + ret = system(cmd); > + if (ret != 0) { > + error_setg(errp, "lvresize returned %d error for '%s'", > + ret, bs->filename); lvresize might display an message on stderr on failure but that's at best going to QEMU's stderr. Any error needs to be captured and put into this error message that's fed back to the user / QMP client. > + } > + > + return ret; > + } > + With regards, Daniel
Thank you for the review. On 3/11/24 19:24, Daniel P. Berrangé wrote: > On Mon, Mar 11, 2024 at 06:40:44PM +0100, Alexander Ivanov wrote: >> If a block device is an LVM logical volume we can resize it using >> standard LVM tools. >> >> In raw_co_truncate() check if the block device is a LV using lvdisplay >> and resize it executing lvresize. >> >> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >> --- >> block/file-posix.c | 27 +++++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >> diff --git a/block/file-posix.c b/block/file-posix.c >> index 35684f7e21..cf8a04e6f7 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -2670,6 +2670,33 @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset, >> if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) { >> int64_t cur_length = raw_getlength(bs); >> >> + /* >> + * Check if the device is an LVM logical volume and try to resize >> + * it using LVM tools. >> + */ >> + if (S_ISBLK(st.st_mode) && offset > 0) { >> + char cmd[PATH_MAX + 32]; >> + >> + snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null", >> + bs->filename); > PATH_MAX + snprint is a bad practice - g_strdup_printf() is recommended > for dynamic allocation, along with g_autofree for release. > >> + ret = system(cmd); > IMHO using 'system' for spawning processes is dubious in any non-trivial > program. > > Historically at least it does not have well defined behaviour wrt signal > handling in the child, before execve is called. ie potentially a signal > handler registered by QEMU in the parent could run in the child having > ill-effects. > > 'system' also executes via the shell which opens up many risks with > unsanitized files path being substituted into the command line. >> + if (ret != 0) { >> + error_setg(errp, "lvdisplay returned %d error for '%s'", >> + ret, bs->filename); >> + return ret; >> + } > Calling 'lvdisplay' doesn't seem to be needed. Surely 'lvresize' is > going to report errors if it isn't an LVM device. The problem is that we don't know if 'lvresize' returned an error because of non-LVM device or there was another reason. For the first variant we should continue original code execution, for the second - return an error. > > If we want to detect an LVM device though, couldn't we lookup > 'device-mapper' in /proc/devices and then major the device major > node number. It will require more code for getting device major, file reading and parsing. Why this way is better? > >> + >> + snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s > /dev/null", >> + offset, bs->filename); >> + ret = system(cmd); >> + if (ret != 0) { >> + error_setg(errp, "lvresize returned %d error for '%s'", >> + ret, bs->filename); > lvresize might display an message on stderr on failure but that's at best > going to QEMU's stderr. Any error needs to be captured and put into > this error message that's fed back to the user / QMP client. It seems I need to implement a high level function for programs execution. Looked at g_spawn_sync(), but it uses fork() under the hood and we could have a performance issue. Haven't found a high level function that uses vfork() and allows to catch stderr. > >> + } >> + >> + return ret; >> + } >> + > With regards, > Daniel
On Tue, Mar 12, 2024 at 06:04:26PM +0100, Alexander Ivanov wrote: > Thank you for the review. > > On 3/11/24 19:24, Daniel P. Berrangé wrote: > > On Mon, Mar 11, 2024 at 06:40:44PM +0100, Alexander Ivanov wrote: > > > If a block device is an LVM logical volume we can resize it using > > > standard LVM tools. > > > > > > In raw_co_truncate() check if the block device is a LV using lvdisplay > > > and resize it executing lvresize. > > > > > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> > > > --- > > > block/file-posix.c | 27 +++++++++++++++++++++++++++ > > > 1 file changed, 27 insertions(+) > > > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > > index 35684f7e21..cf8a04e6f7 100644 > > > --- a/block/file-posix.c > > > +++ b/block/file-posix.c > > > @@ -2670,6 +2670,33 @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset, > > > if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) { > > > int64_t cur_length = raw_getlength(bs); > > > + /* > > > + * Check if the device is an LVM logical volume and try to resize > > > + * it using LVM tools. > > > + */ > > > + if (S_ISBLK(st.st_mode) && offset > 0) { > > > + char cmd[PATH_MAX + 32]; > > > + > > > + snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null", > > > + bs->filename); > > PATH_MAX + snprint is a bad practice - g_strdup_printf() is recommended > > for dynamic allocation, along with g_autofree for release. > > > > > + ret = system(cmd); > > IMHO using 'system' for spawning processes is dubious in any non-trivial > > program. > > > > Historically at least it does not have well defined behaviour wrt signal > > handling in the child, before execve is called. ie potentially a signal > > handler registered by QEMU in the parent could run in the child having > > ill-effects. > > > > 'system' also executes via the shell which opens up many risks with > > unsanitized files path being substituted into the command line. > > > + if (ret != 0) { > > > + error_setg(errp, "lvdisplay returned %d error for '%s'", > > > + ret, bs->filename); > > > + return ret; > > > + } > > Calling 'lvdisplay' doesn't seem to be needed. Surely 'lvresize' is > > going to report errors if it isn't an LVM device. > The problem is that we don't know if 'lvresize' returned an error because of > non-LVM device or there was another reason. For the first variant we should > continue original code execution, for the second - return an error. > > > > If we want to detect an LVM device though, couldn't we lookup > > 'device-mapper' in /proc/devices and then major the device major > > node number. > It will require more code for getting device major, file reading and > parsing. > Why this way is better? There are a variety of reasons why these LVM commands could fail. Directly detecting whether or not this is an LVM device, rather than inferring it from 'lvdisplay' result is a more precise check which will allow for clearer error reporting IMHO. Ideally we wouldn't even spawn a process for the resize operation, but instead call into the device mapper library API. That would let it work even when the seccomp sandbox is enabled and blocking process spawning. IME the device mapper API is not too friendly though, so I didn't want to suggest that as a blocker. > > > + snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s > /dev/null", > > > + offset, bs->filename); > > > + ret = system(cmd); > > > + if (ret != 0) { > > > + error_setg(errp, "lvresize returned %d error for '%s'", > > > + ret, bs->filename); > > lvresize might display an message on stderr on failure but that's at best > > going to QEMU's stderr. Any error needs to be captured and put into > > this error message that's fed back to the user / QMP client. > It seems I need to implement a high level function for programs execution. > Looked at > g_spawn_sync(), but it uses fork() under the hood and we could have a > performance > issue. Haven't found a high level function that uses vfork() and allows to > catch stderr. GLib should be using posix_spawn internally on all modern platforms. Resizing LVM volumes does not sound like a hot code path, so I'm seeing what performance concerns we would have. With regards, Daniel
diff --git a/block/file-posix.c b/block/file-posix.c index 35684f7e21..cf8a04e6f7 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2670,6 +2670,33 @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset, if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) { int64_t cur_length = raw_getlength(bs); + /* + * Check if the device is an LVM logical volume and try to resize + * it using LVM tools. + */ + if (S_ISBLK(st.st_mode) && offset > 0) { + char cmd[PATH_MAX + 32]; + + snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null", + bs->filename); + ret = system(cmd); + if (ret != 0) { + error_setg(errp, "lvdisplay returned %d error for '%s'", + ret, bs->filename); + return ret; + } + + snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s > /dev/null", + offset, bs->filename); + ret = system(cmd); + if (ret != 0) { + error_setg(errp, "lvresize returned %d error for '%s'", + ret, bs->filename); + } + + return ret; + } + if (offset != cur_length && exact) { error_setg(errp, "Cannot resize device files"); return -ENOTSUP;
If a block device is an LVM logical volume we can resize it using standard LVM tools. In raw_co_truncate() check if the block device is a LV using lvdisplay and resize it executing lvresize. Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> --- block/file-posix.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)