[PATCH] overlayfs: Do d_type check only if work dir creation was successful

[PATCH] overlayfs: Do d_type check only if work dir creation was successful

[Vivek Goyal]

d_type check requires successful creation of workdir as iterates
through work dir and expects work dir to be present in it. If that's
not the case, this check will always return d_type not supported even
if underlying filesystem might be supporting it.

So don't do this check if work dir creation failed in previous step.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/super.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 791235e..48234425 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1064,16 +1064,19 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		/*
 		 * Upper should support d_type, else whiteouts are visible.
 		 * Given workdir and upper are on same fs, we can do
-		 * iterate_dir() on workdir.
+		 * iterate_dir() on workdir. This check requires successful
+		 * creation of workdir in previous step.
 		 */
-		err = ovl_check_d_type_supported(&workpath);
-		if (err < 0)
-			goto out_put_workdir;
+		if (ufs->workdir) {
+			err = ovl_check_d_type_supported(&workpath);
+			if (err < 0)
+				goto out_put_workdir;
 
-		if (!err) {
-			pr_err("overlayfs: upper fs needs to support d_type.\n");
-			err = -EINVAL;
-			goto out_put_workdir;
+			if (!err) {
+				pr_err("overlayfs: upper fs needs to support d_type.\n");
+				err = -EINVAL;
+				goto out_put_workdir;
+			}
 		}
 	}
 
-- 
2.7.4

Re: [PATCH] overlayfs: Do d_type check only if work dir creation was successful

[Carlos Maiolino]

Hi Vivek.

On Fri, May 20, 2016 at 09:04:26AM -0400, Vivek Goyal wrote:
> d_type check requires successful creation of workdir as iterates
> through work dir and expects work dir to be present in it. If that's
> not the case, this check will always return d_type not supported even
> if underlying filesystem might be supporting it.
>

ovl_check_d_type_supported, will call iterate_dir(), which is supposed to return
-ENOENT if the work dir is not present AFAICT, isn't that enough to detect if
work dir is present and handle it accordingly?

I recently started to learn overlay code, so my apologies if my above comment is
not true.

Cheers

> So don't do this check if work dir creation failed in previous step.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/super.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 791235e..48234425 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1064,16 +1064,19 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  		/*
>  		 * Upper should support d_type, else whiteouts are visible.
>  		 * Given workdir and upper are on same fs, we can do
> -		 * iterate_dir() on workdir.
> +		 * iterate_dir() on workdir. This check requires successful
> +		 * creation of workdir in previous step.
>  		 */
> -		err = ovl_check_d_type_supported(&workpath);
> -		if (err < 0)
> -			goto out_put_workdir;
> +		if (ufs->workdir) {
> +			err = ovl_check_d_type_supported(&workpath);
> +			if (err < 0)
> +				goto out_put_workdir;
>  
> -		if (!err) {
> -			pr_err("overlayfs: upper fs needs to support d_type.\n");
> -			err = -EINVAL;
> -			goto out_put_workdir;
> +			if (!err) {
> +				pr_err("overlayfs: upper fs needs to support d_type.\n");
> +				err = -EINVAL;
> +				goto out_put_workdir;
> +			}
>  		}
>  	}
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

Re: [PATCH] overlayfs: Do d_type check only if work dir creation was successful

[Vivek Goyal]

On Mon, May 23, 2016 at 02:01:50PM +0200, Carlos Maiolino wrote:
> Hi Vivek.
> 
> On Fri, May 20, 2016 at 09:04:26AM -0400, Vivek Goyal wrote:
> > d_type check requires successful creation of workdir as iterates
> > through work dir and expects work dir to be present in it. If that's
> > not the case, this check will always return d_type not supported even
> > if underlying filesystem might be supporting it.
> >
> 
> ovl_check_d_type_supported, will call iterate_dir(), which is supposed to return
> -ENOENT if the work dir is not present AFAICT, isn't that enough to detect if
> work dir is present and handle it accordingly?

Hi Carlos,

So there is directory "work" under user specified workdir. Say user
specified "overlaywork" as workdir, then we are listing contents of "
overlaywork" and which should find "work" underneath it. Even if "work"
creation failed, "." and ".." are found and returned and their types
are returned as DT_DIR (despite the fact that filesystem does not support
d_type").

So in short, I am not getting -ENOENT if work dir creation failed.

Thanks
Vivek


> 
> I recently started to learn overlay code, so my apologies if my above comment is
> not true.
> 
> Cheers
> 
> > So don't do this check if work dir creation failed in previous step.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/super.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 791235e..48234425 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -1064,16 +1064,19 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> >  		/*
> >  		 * Upper should support d_type, else whiteouts are visible.
> >  		 * Given workdir and upper are on same fs, we can do
> > -		 * iterate_dir() on workdir.
> > +		 * iterate_dir() on workdir. This check requires successful
> > +		 * creation of workdir in previous step.
> >  		 */
> > -		err = ovl_check_d_type_supported(&workpath);
> > -		if (err < 0)
> > -			goto out_put_workdir;
> > +		if (ufs->workdir) {
> > +			err = ovl_check_d_type_supported(&workpath);
> > +			if (err < 0)
> > +				goto out_put_workdir;
> >  
> > -		if (!err) {
> > -			pr_err("overlayfs: upper fs needs to support d_type.\n");
> > -			err = -EINVAL;
> > -			goto out_put_workdir;
> > +			if (!err) {
> > +				pr_err("overlayfs: upper fs needs to support d_type.\n");
> > +				err = -EINVAL;
> > +				goto out_put_workdir;
> > +			}
> >  		}
> >  	}
> >  
> > -- 
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Carlos

Re: [PATCH] overlayfs: Do d_type check only if work dir creation was successful

[Carlos Maiolino]

On Mon, May 23, 2016 at 10:04:12AM -0400, Vivek Goyal wrote:
> On Mon, May 23, 2016 at 02:01:50PM +0200, Carlos Maiolino wrote:
> > Hi Vivek.
> > 
> > On Fri, May 20, 2016 at 09:04:26AM -0400, Vivek Goyal wrote:
> > > d_type check requires successful creation of workdir as iterates
> > > through work dir and expects work dir to be present in it. If that's
> > > not the case, this check will always return d_type not supported even
> > > if underlying filesystem might be supporting it.
> > >
> > 
> > ovl_check_d_type_supported, will call iterate_dir(), which is supposed to return
> > -ENOENT if the work dir is not present AFAICT, isn't that enough to detect if
> > work dir is present and handle it accordingly?
> 
> Hi Carlos,
> 
> So there is directory "work" under user specified workdir. Say user
> specified "overlaywork" as workdir, then we are listing contents of "
> overlaywork" and which should find "work" underneath it. Even if "work"
> creation failed, "." and ".." are found and returned and their types
> are returned as DT_DIR (despite the fact that filesystem does not support
> d_type").
> 
> So in short, I am not getting -ENOENT if work dir creation failed.
> 
> Thanks
> Vivek
> 

Hi Vivek, my apologies for my delay, I had some problems with my mail filter and
I missed your reply before.

Thanks for the explanation, it makes sense to me, you can add:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

Cheers

-- 
Carlos