[PATCHES] fix flapping offline flag in gpfs module

Michael Adam obnox at samba.org
Wed Jul 9 16:38:43 MDT 2014


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.

  - 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?

  - 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.  ;-)
-------------- next part --------------
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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140710/6c8935cf/attachment.pgp>


More information about the samba-technical mailing list