Re: [PATCH] ovl: resolve more conflicting mount options

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Apr 09, 2020 at 07:39:02PM +0300, Amir Goldstein wrote:
> Similar to the way that a conflict between metacopy=on,redirect_dir=off
> is resolved, also resolve conflicts between nfs_export=on,index=off and
> nfs_export=on,metacopy=on.
> 
> An explicit mount option wins over a default config value.
> Both explicit mount options result in an error.
> 
> Without this change the xfstests group overlay/exportfs are skipped if
> metacopy is enabled by default.
> 
> Reported-by: Chengguang Xu <cgxu519@mykernel.net>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  Documentation/filesystems/overlayfs.rst |  7 ++--
>  fs/overlayfs/super.c                    | 48 +++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index c9d2bf96b02d..660dbaf0b9b8 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -365,8 +365,8 @@ pointed by REDIRECT. This should not be possible on local system as setting
>  "trusted." xattrs will require CAP_SYS_ADMIN. But it should be possible
>  for untrusted layers like from a pen drive.
>  
> -Note: redirect_dir={off|nofollow|follow[*]} conflicts with metacopy=on, and
> -results in an error.
> +Note: redirect_dir={off|nofollow|follow[*]} and nfs_export=on mount options
> +conflict with metacopy=on, and will result in an error.
>  
>  [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
>  given.
> @@ -560,6 +560,9 @@ When the NFS export feature is enabled, all directory index entries are
>  verified on mount time to check that upper file handles are not stale.
>  This verification may cause significant overhead in some cases.
>  
> +Note: the mount options index=off,nfs_export=on are conflicting and will
> +result in an error.
> +
>  
>  Testsuite
>  ---------
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 732ad5495c92..fbd6207acdbf 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -470,6 +470,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  	char *p;
>  	int err;
>  	bool metacopy_opt = false, redirect_opt = false;
> +	bool nfs_export_opt = false, index_opt = false;
>  
>  	config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
>  	if (!config->redirect_mode)
> @@ -519,18 +520,22 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  
>  		case OPT_INDEX_ON:
>  			config->index = true;
> +			index_opt = true;
>  			break;
>  
>  		case OPT_INDEX_OFF:
>  			config->index = false;
> +			index_opt = true;
>  			break;
>  
>  		case OPT_NFS_EXPORT_ON:
>  			config->nfs_export = true;
> +			nfs_export_opt = true;
>  			break;
>  
>  		case OPT_NFS_EXPORT_OFF:
>  			config->nfs_export = false;
> +			nfs_export_opt = true;
>  			break;
>  
>  		case OPT_XINO_ON:
> @@ -552,6 +557,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  
>  		case OPT_METACOPY_OFF:
>  			config->metacopy = false;
> +			metacopy_opt = true;

Hi Amir,

I am wondering why metacopy_opt needs to be set for OPT_METACOPY_OFF case.
In this case config->metacopy=false and it does not conflict with
config->nfs_export at all. So there is no need to know if metacopy=off
was specified as mount option or not.

Vivek

>  			break;
>  
>  		default:
> @@ -601,6 +607,48 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  		}
>  	}
>  
> +	/* Resolve nfs_export -> index dependency */
> +	if (config->nfs_export && !config->index) {
> +		if (nfs_export_opt && index_opt) {
> +			pr_err("conflicting options: nfs_export=on,index=off\n");
> +			return -EINVAL;
> +		}
> +		if (index_opt) {
> +			/*
> +			 * There was an explicit index=off that resulted
> +			 * in this conflict.
> +			 */
> +			pr_info("disabling nfs_export due to index=off\n");
> +			config->nfs_export = false;
> +		} else {
> +			/* Automatically enable index otherwise. */
> +			config->index = true;
> +		}
> +	}
> +
> +	/* Resolve nfs_export -> !metacopy dependency */
> +	if (config->nfs_export && config->metacopy) {
> +		if (nfs_export_opt && metacopy_opt) {
> +			pr_err("conflicting options: nfs_export=on,metacopy=on\n");
> +			return -EINVAL;
> +		}
> +		if (metacopy_opt) {
> +			/*
> +			 * There was an explicit metacopy=on that resulted
> +			 * in this conflict.
> +			 */
> +			pr_info("disabling nfs_export due to metacopy=on\n");
> +			config->nfs_export = false;
> +		} else {
> +			/*
> +			 * There was an explicit nfs_export=on that resulted
> +			 * in this conflict.
> +			 */
> +			pr_info("disabling metacopy due to nfs_export=on\n");
> +			config->metacopy = false;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.17.1
>