Re: [PATCH v3 2/5] ovl: relax same fs constrain for constant st_ino

[Chandan Rajendra <chandan@linux.vnet.ibm.com>]

On Thursday, June 1, 2017 2:32:56 PM IST Amir Goldstein wrote:
> For the case of all layers not on the same fs, use the copy up
> origin st_ino/st_dev for non-dir, if we know it.
> 
> This guarantied constant and persistent st_ino/st_dev for non-dir,
> but not system-wide uniqueness, like in the same fs case.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/inode.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index d613e2c41242..5f285c179bbd 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -65,6 +65,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>  	struct path realpath;
>  	const struct cred *old_cred;
>  	bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
> +	bool samefs = ovl_same_sb(dentry->d_sb);
>  	int err;
> 
>  	type = ovl_path_real(dentry, &realpath);
> @@ -74,16 +75,16 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>  		goto out;
> 
>  	/*
> -	 * When all layers are on the same fs, all real inode number are
> -	 * unique, so we use the overlay st_dev, which is friendly to du -x.
> -	 *
> -	 * We also use st_ino of the copy up origin, if we know it.
> -	 * This guaranties constant st_dev/st_ino across copy up.
> +	 * For non-dir or same fs, we use st_ino of the copy up origin, if we
> +	 * know it. This guaranties constant st_dev/st_ino across copy up.
>  	 *
>  	 * If filesystem supports NFS export ops, this also guaranties
>  	 * persistent st_ino across mount cycle.
> +	 *
> +	 * When all layers are on the same fs, all real inode number are
> +	 * unique, so we use the overlay st_dev, which is friendly to du -x.
>  	 */
> -	if (ovl_same_sb(dentry->d_sb)) {
> +	if (!is_dir || samefs) {
>  		if (OVL_TYPE_ORIGIN(type)) {
>  			struct kstat lowerstat;
>  			u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
> @@ -94,7 +95,6 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>  			if (err)
>  				goto out;
> 
> -			WARN_ON_ONCE(stat->dev != lowerstat.dev);
>  			/*
>  			 * Lower hardlinks are broken on copy up to different
>  			 * upper files, so we cannot use the lower origin st_ino
> @@ -102,17 +102,23 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>  			 */
>  			if (is_dir || lowerstat.nlink == 1)
>  				stat->ino = lowerstat.ino;
> +
> +			if (samefs)
> +				WARN_ON_ONCE(stat->dev != lowerstat.dev);
> +			else
> +				stat->dev = lowerstat.dev;
>  		}
> -		stat->dev = dentry->d_sb->s_dev;
> -	} else if (is_dir) {
> +		if (samefs)
> +			stat->dev = dentry->d_sb->s_dev;
> +	} else {
>  		/*
> -		 * If not all layers are on the same fs the pair {real st_ino;
> -		 * overlay st_dev} is not unique, so use the non persistent
> -		 * overlay st_ino.
> -		 *
>  		 * Always use the overlay st_dev for directories, so 'find
>  		 * -xdev' will scan the entire overlay mount and won't cross the
>  		 * overlay mount boundaries.
> +		 *
> +		 * If not all layers are on the same fs the pair {real st_ino;
> +		 * overlay st_dev} is not unique, so use the non persistent
> +		 * overlay st_ino for directories.
>  		 */
>  		stat->dev = dentry->d_sb->s_dev;
>  		stat->ino = dentry->d_inode->i_ino;
> 

Hi Amir,

The following question is not directly related to your patch. 

With this patch applied, we have the following code snippet inside
ovl_getattr(),

       if (!is_dir || samefs) {
                if (OVL_TYPE_ORIGIN(type)) {
                        struct kstat lowerstat;
                        u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);

                        ovl_path_lower(dentry, &realpath);
                        err = vfs_getattr(&realpath, &lowerstat,
                                          lowermask, flags);
                        if (err)
                                goto out;

When invoking vfs_getattr() on a non-dir file, the code assumes that the
copy-up origin inode always exists in lowerdir. Are we assuming that after an
overlayfs filesystem has been unmounted, the lower filesystem should never be
manipulated (e.g. deleting the file which was the copy-up origin)? If the
copy-up origin file gets deleted, On a later mount of the overlayfs
filesystem, calls to stat(2) for the corresponding upperdir file will fail
because of vfs_getattr() returning an error code.

-- 
chandan