Re: [PATCH] qga: skip bind mounts in fs list

[Jean-Louis Dupond <jean-louis@dupond.be>]

On 2/10/2024 12:06, Jean-Louis Dupond wrote:
> The filesystem list in build_fs_mount_list should skip bind mounts.
> This because we end up in locking situations when doing fsFreeze. Like
> mentioned in [1] and [2].
>
> Next to that, the build_fs_mount_list call did a fallback via
> build_fs_mount_list_from_mtab if mountinfo did not exist.
> There it skipped bind mounts, but this is broken for newer OS.
> This as mounts does not return the path of the bind mount but the
> underlying dev/partition, so S_ISDIR will never return true in
> dev_major_minor call.
>
> This patch simply checks the existing devmajor:devminor tuple in the
> mounts, and if it already exists, this means we have the same devices
> mounted again, a bind mount. So skip this.
>
> Same approach is used in open-vm-tools [3].
>
> [1]: https://gitlab.com/qemu-project/qemu/-/issues/592
> [2]: https://gitlab.com/qemu-project/qemu/-/issues/520
> [3]: https://github.com/vmware/open-vm-tools/commit/d58847b497e212737007958c945af1df22a8ab58
>
> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
> ---
>   qga/commands-linux.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
>
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> index 51d5e3d927..426b040ab8 100644
> --- a/qga/commands-linux.c
> +++ b/qga/commands-linux.c
> @@ -59,6 +59,22 @@ static int dev_major_minor(const char *devpath,
>       return -1;
>   }
>   
> +/*
> + * Check if we already have the devmajor:devminor in the mounts
> + * If thats the case return true.
> + */
> +static bool dev_exists(FsMountList *mounts, unsigned int devmajor, unsigned int devminor)
> +{
> +    FsMount *mount;
> +
> +    QTAILQ_FOREACH(mount, mounts, next) {
> +        if (mount->devmajor == devmajor && mount->devminor == devminor) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
>   static bool build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
>   {
>       struct mntent *ment;
> @@ -89,6 +105,10 @@ static bool build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
>               /* Skip bind mounts */
>               continue;
>           }
> +        if (dev_exists(mounts, devmajor, devminor)) {
> +            /* Skip already existing devices (bind mounts) */
> +            continue;
> +        }
>   
>           mount = g_new0(FsMount, 1);
>           mount->dirname = g_strdup(ment->mnt_dir);
> @@ -172,6 +192,11 @@ bool build_fs_mount_list(FsMountList *mounts, Error **errp)
>               }
>           }
>   
> +        if (dev_exists(mounts, devmajor, devminor)) {
> +            /* Skip already existing devices (bind mounts) */
> +            continue;
> +        }
> +
>           mount = g_new0(FsMount, 1);
>           mount->dirname = g_strdup(line + dir_s);
>           mount->devtype = g_strdup(dash + type_s);


Ping + add kkostiuk@redhat.com as I missed him in the initial mail.