[PATCHES] fix flapping offline flag in gpfs module

[Christof Schmitt]

Hi Michael,

On Thu, Jul 10, 2014 at 12:38:43AM +0200, Michael Adam wrote:
> There is this awful problem of the flapping offline flag
> when using the gpfs module and "gpfs:winattr = yes".
> It was pretty clear since a time that this must be due
> to uninitialized data (where the winattrs are stored),
> i.e. the vfs_private in the struct stat_ex.
> 
> I was recently able to debug this and found two causes
> of uninitialized data:
> 
> - One in vfs_gpfs_fstat, where it was imply forgotten to copy the
>   result of smbd_fget_gpfs_winattrs().
>   This one is present in the code since Samba 3.6 and easily fixed.
> 
> - The other is much more subtle:
>   in directory enumeration, commit
>   2a65e8befef004fd18d17853a1b72155752346c8 introduced an
>   optimization which made vfswrap_readdir() call
>   fstatat directly if available, marking the stat buffer valid.
>   The problem with this is that this does not go through vfs
>   and hence, (e.g.) the gpfs vfs module has no chance to
>   fill the vfs_private with valid info.
>   Hence the is_offline() function can not rely on the
>   vfs_private info if there is a valid stat buffer.
> 
>   - The fix attached simply ignores the stat buffer in
>     is_offline() an always calls get_gpfs_winattrs.
>     thereby at least partly sacrificing the optimization.
> 
>   - A proper fix might add fstatat to the vfs and
>     have vfs_gpfs implement it.
>     Should we do that? or leave it at the above fix?
>     I think we need the above fix in any case, since
>     we will want to port it to 4.1.

Yes, i agree that we need to fix this issue first, and then figure out
how to improve the code.

>   - I also added a patch to log a message when gpfs_is_offline()
>     detects a potential case of uninitialized buffer.
>     Maybe debug level 0 is too low?

Debug level 0 should be good. It should never happen, but if it does, we
want to know about it.

>   - finally, I added a patch that initializes the
>     stat buffer to 0 in directory enumeration, so
>     that after fstatat, removing the randomness in
>     failure of vfs_private to be filled by the vfs
>     modules.
> 
> Comments / reviews / push appreciated.
> 
> Thanks - Michael
> 
> PS: This is a very long mail, and very long commit messages
> for patches with very small diff, but I think this is important.  ;-)

Reviewed-by: Christof Schmitt <cs at samba.org>

> From 7fc76c8c135183d03b1d4c61842ad82212759658 Mon Sep 17 00:00:00 2001
> From: Michael Adam <obnox at samba.org>
> Date: Thu, 3 Jul 2014 10:07:37 +0200
> Subject: [PATCH 1/4] s3:vfs:gpfs: store the winAttrs in the struct_ex when we
>  got them in vfs_gpfs_fstat()
> 
> This may (e.g.) have lead to some occurrences of flapping offline bits.
> 
> Signed-off-by: Michael Adam <obnox at samba.org>
> ---
>  source3/modules/vfs_gpfs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> index 5ad2595..d8d59f9 100644
> --- a/source3/modules/vfs_gpfs.c
> +++ b/source3/modules/vfs_gpfs.c
> @@ -1622,6 +1622,7 @@ static int vfs_gpfs_fstat(struct vfs_handle_struct *handle,
>  		sbuf->st_ex_calculated_birthtime = false;
>  		sbuf->st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
>  		sbuf->st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
> +		sbuf->vfs_private = attrs.winAttrs;
>  	}
>  	return 0;
>  }
> -- 
> 1.9.1
> 
> 
> From 63d2b2a3d987f46778d94f879cbae809d2fd45a8 Mon Sep 17 00:00:00 2001
> From: Michael Adam <obnox at samba.org>
> Date: Thu, 3 Jul 2014 10:10:11 +0200
> Subject: [PATCH 2/4] s3:vfs:gpfs: fix flapping offline: always get winAttrs
>  from gpfs for is_offline
> 
> There is a problem of flapping offline due to uninitialized
> stat buffers. Due to a optimization in vfswrap_readdir which
> directly calling fastatat (i.e. not through vfs), marking the
> stat buffer valid, there is nothing this module can do about
> it and hence can not currently not rely on the vaildity of
> the stat buffer.
> 
> By always calling out to GPFS even when the stat buffer is
> flagged valid, we can always return correct offline information,
> thereby sacrificing the readdir optimization.
> 
> Pair-Programmed-With: Volker Lendecke <vl at samba.org>
> 
> Signed-off-by: Michael Adam <obnox at samba.org>
> ---
>  source3/modules/vfs_gpfs.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> index d8d59f9..9fcce78 100644
> --- a/source3/modules/vfs_gpfs.c
> +++ b/source3/modules/vfs_gpfs.c
> @@ -1819,9 +1819,7 @@ static bool vfs_gpfs_is_offline(struct vfs_handle_struct *handle,
>  		return -1;
>  	}
>  
> -	if (VALID_STAT(*sbuf)) {
> -		attrs.winAttrs = sbuf->vfs_private;
> -	} else {
> +	{
>  		int ret;
>  		ret = get_gpfs_winattrs(path, &attrs);
>  
> -- 
> 1.9.1
> 
> 
> From 8f44883db94314c007c197927a9dd0809076754d Mon Sep 17 00:00:00 2001
> From: Michael Adam <obnox at samba.org>
> Date: Wed, 9 Jul 2014 23:56:34 +0200
> Subject: [PATCH 3/4] s3:vfs:gpfs: log when winAttr-garbage is detected (by
>  heuristics) in is_offline
> 
> In is_offline(), check whether the winAttrs are filled with bits
> outside 0xFFFF and log it prominently: Since GPFS only
> fills 0xFFFF, this could be due to an uninitialized buffer
> (or another vfs module filling vfs_private? ...).
> 
> Signed-off-by: Michael Adam <obnox at samba.org>
> ---
>  source3/modules/vfs_gpfs.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> index 9fcce78..c94e4e8 100644
> --- a/source3/modules/vfs_gpfs.c
> +++ b/source3/modules/vfs_gpfs.c
> @@ -1819,6 +1819,11 @@ static bool vfs_gpfs_is_offline(struct vfs_handle_struct *handle,
>  		return -1;
>  	}
>  
> +	if (VALID_STAT(*sbuf) && (sbuf->vfs_private & ~0x0FFFF) != 0) {
> +		DEBUG(0, ("vfs_gpfs_is_offline: valid stat but "
> +			  "uninitialized winAttrs (0x%08x)?\n",
> +			  (uint32_t)sbuf->vfs_private));
> +	}
>  	{
>  		int ret;
>  		ret = get_gpfs_winattrs(path, &attrs);
> -- 
> 1.9.1
> 
> 
> From a6198420a0e9148714733a5b7854ef4d84c5e3a8 Mon Sep 17 00:00:00 2001
> From: Michael Adam <obnox at samba.org>
> Date: Thu, 3 Jul 2014 10:00:13 +0200
> Subject: [PATCH 4/4] s3:smbd: initialize stat_ex buffer in
>  smbd_dirptr_get_entry()
> 
> This prevents random garbage in the vfs_private member.
> Usually it should not be a problem to leave initialization
> of the vfs_private to the vfs module who wants to use it,
> but further down in the directory listing code, in
> vfswrap_readdir, there is in optimization introduced
> with 2a65e8befef004fd18d17853a1b72155752346c8, to call
> fstatat if possible to already fill stat info in the
> readdir call.
> 
> The problem is that this calls fstatat directly,
> not going through VFS, but still making the stat buffer
> valid, leaving vfs_private with random garbage.
> Hence a vfs module using vfs_private, like vfs_gpfs
> does for offline info (and winAttrs in general)
> does not have a chance to tell whether the vfs_private
> is valid if the stat buffer is marked valid.
> This is a reason for the "flapping offline flag" problem
> of the vfs_gpfs module.
> 
> Initializing the vfs_private to 0 here will for the
> vfs_gpfs module result in files being marked online
> always in a directory listing. So this is not a real fix
> but it does at least make the problem less random.
> 
> A real general fix might be to implement SMB_VFS_FSTATAT()
> and use it here.
> 
> Signed-off-by: Michael Adam <obnox at samba.org>
> ---
>  source3/smbd/dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
> index 038281e..818f778 100644
> --- a/source3/smbd/dir.c
> +++ b/source3/smbd/dir.c
> @@ -1126,7 +1126,7 @@ bool smbd_dirptr_get_entry(TALLOC_CTX *ctx,
>  	while (true) {
>  		long cur_offset;
>  		long prev_offset;
> -		SMB_STRUCT_STAT sbuf;
> +		SMB_STRUCT_STAT sbuf = { 0 };
>  		char *dname = NULL;
>  		bool isdots;
>  		char *fname = NULL;
> -- 
> 1.9.1
>