[PATCHES] fix flapping offline flag in gpfs module

Michael Adam obnox at samba.org
Thu Jul 17 16:19:00 MDT 2014


Hi Christof,

On 2014-07-10 at 16:11 -0700, Christof Schmitt wrote:
> On Fri, Jul 11, 2014 at 12:22:35AM +0200, Michael Adam wrote:
> > On 2014-07-10 at 10:46 -0700, Christof Schmitt wrote:
> > > On Thu, Jul 10, 2014 at 10:54:30AM +0200, Volker Lendecke wrote:
> > > > 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.
> > > > 
> > > > As discussed offline with Michael: I think with patch 2/4
> > > > vfs_private is not referenced at all anymore. vfs_private is
> > > > a broken concept once more than one VFS module starts using
> > > > it. IMHO it should go.
> > > 
> > > Other parts of the vfs_gpfs module still access the vfs_private.
> > 
> > Right, but I think that most of these accesses are potentially
> > invalid, except for the ones setting it: They all check for
> > (or reset) the offline flag. Hence they all may suffer from
> > the same problem as the one in is_offline: They may not operate
> > on properly intialized data. Some don't even check for
> > VALID_STAT().
> > 
> > They should probably use SMB_VFS_IS_OFFLINE() (and
> > SMB_VFS_SET_OFFLINE()).
> > And then vfs_private could easily be removed.
> 
> Yes. The question is if this affects performance, but it is certainly
> better to be correct first and then optimize later.
> 
> > Thinking about it: these uses of the offline check
> > are more or less exactly the same as in the tsmsm module.
> > (Methods: aio_force, sendfile, pread, preade_recv, pwrite,
> > pwrite_recv, only the open meth is additional in gpfs).
> 
> Even the check in open is not tied to GPFS. It is just meant to prevent
> mass-recalls of files from clients that do not check the offline flag.
> If we move the checks to vfs_hsm, then we should move this one, too.
> 
> > All this is generic treatment of a hsm system.
> > There seems to be nothing special in this, nor in
> > tsmsm. So, wouldn't it be a good improvement to
> > extract this logic from vfs_gpfs and vfs_tsmsm
> > to a new module, say vfs_hsm. Then only the detection
> > (and setting) of offline files would remain specific
> > to vfs_gpfs and others.
> > 
> > Does this sound like a plan?
> 
> Yes, there is certainly duplicated code that should be avoided. I assume
> that the new module would call SMB_VFS_IS_OFFLINE and that call is
> then intercepted by vfs_gpfs or vfs_tsmsm.
> 
> Are you going to write a patch? :-)

Attached find (as a first step) a patcheset that removes
the use of stat_ex.vfs_private from vfs_gpfs by calling
SMB_VFS_IS_OFFLINE() as discussed above

I guess that this also fixes a few more errors regarding
reaction to of assumed offline status in the module.

Review/push appreciated.

Next step:

We could move forward and remove the vfs_private from
stat_ex altogether in a next step. Attached find a second
patch to do this. But in principle, this stat_ex with
the vfs_private member belongs to the vfs. ==> Do we need to
bump the vfs version for this?
And while this is a broken concept (vfs_private potentially
being shared by multiple modules), and not used any more
in our upstream modules, I still wanted to heare comments
wheter we want to do it: There _might_ be users of it out
there...

Opinions?

Further steps: extract the hsm-logic using is_offline()
from vfs_gpfs and vfs_tsmsm to one universally useful module vfs_hsm.
Will happily code this but might take a little time  until I
get to really doing it.

Cheers - Michael

-------------- next part --------------
From 35465a370d19893c4d0121601e3741a346f91951 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 10 Jul 2014 13:30:51 +0200
Subject: [PATCH 1/4] Revert "s3:vfs:gpfs: log when winAttr-garbage is detected
 (by heuristics) in is_offline"

This reverts commit 8f44883db94314c007c197927a9dd0809076754d.

The next commits will be removing all access to stat_ex.vfs_private from
vfs_gpfs. This revert of the last addition is a preparation.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/modules/vfs_gpfs.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
index c94e4e8..9fcce78 100644
--- a/source3/modules/vfs_gpfs.c
+++ b/source3/modules/vfs_gpfs.c
@@ -1819,11 +1819,6 @@ 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 9b4b958c5073be27e609697f5c29072a718e89d4 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 17 Jul 2014 17:22:09 +0200
Subject: [PATCH 2/4] s3:vfs:gpfs: Remove all reading uses of
 stat_ex.vfs_private from vfs_gfs.

This was used as a cache for offline-info in the stat buffer.
But as the implementatio of gpfs_is_offline() showed, this cache
does not always carry valid info when the stat itself is valid
(since at least one call goes to fstatat() directly, circumventing
 the vfs).

So the correct thing is to always call SMB_VFS_IS_OFFLINE()
when checking whether a file is offline.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/modules/vfs_gpfs.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
index 9fcce78..54cdd32 100644
--- a/source3/modules/vfs_gpfs.c
+++ b/source3/modules/vfs_gpfs.c
@@ -1848,7 +1848,8 @@ static ssize_t vfs_gpfs_sendfile(vfs_handle_struct *handle, int tofd,
 				 files_struct *fsp, const DATA_BLOB *hdr,
 				 off_t offset, size_t n)
 {
-	if ((fsp->fsp_name->st.vfs_private & GPFS_WINATTR_OFFLINE) != 0) {
+	if (SMB_VFS_IS_OFFLINE(handle->conn, fsp->fsp_name, &fsp->fsp_name->st))
+	{
 		errno = ENOSYS;
 		return -1;
 	}
@@ -2115,8 +2116,8 @@ static int vfs_gpfs_open(struct vfs_handle_struct *handle,
 				return -1);
 
 	if (config->hsm && !config->recalls) {
-		if (VALID_STAT(smb_fname->st) &&
-		    (smb_fname->st.vfs_private & GPFS_WINATTR_OFFLINE)) {
+		if (SMB_VFS_IS_OFFLINE(handle->conn, smb_fname, &smb_fname->st))
+		{
 			DEBUG(10, ("Refusing access to offline file %s\n",
 				  fsp_str_dbg(fsp)));
 			errno = EACCES;
@@ -2137,11 +2138,9 @@ static ssize_t vfs_gpfs_pread(vfs_handle_struct *handle, files_struct *fsp,
 
 	ret = SMB_VFS_NEXT_PREAD(handle, fsp, data, n, offset);
 
-	DEBUG(10, ("vfs_private = %x\n",
-		   (unsigned int)fsp->fsp_name->st.vfs_private));
-
 	if ((ret != -1) &&
-	    ((fsp->fsp_name->st.vfs_private & GPFS_WINATTR_OFFLINE) != 0)) {
+	    SMB_VFS_IS_OFFLINE(handle->conn, fsp->fsp_name, &fsp->fsp_name->st))
+	{
 		fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
 		notify_fname(handle->conn, NOTIFY_ACTION_MODIFIED,
 			     FILE_NOTIFY_CHANGE_ATTRIBUTES,
@@ -2206,11 +2205,9 @@ static ssize_t vfs_gpfs_pread_recv(struct tevent_req *req, int *err)
 	}
 	*err = state->err;
 
-	DEBUG(10, ("vfs_private = %x\n",
-		   (unsigned int)fsp->fsp_name->st.vfs_private));
-
 	if ((state->ret != -1) &&
-	    ((fsp->fsp_name->st.vfs_private & GPFS_WINATTR_OFFLINE) != 0)) {
+	    SMB_VFS_IS_OFFLINE(fsp->conn, fsp->fsp_name, &fsp->fsp_name->st))
+	{
 		fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
 		DEBUG(10, ("sending notify\n"));
 		notify_fname(fsp->conn, NOTIFY_ACTION_MODIFIED,
@@ -2228,11 +2225,9 @@ static ssize_t vfs_gpfs_pwrite(vfs_handle_struct *handle, files_struct *fsp,
 
 	ret = SMB_VFS_NEXT_PWRITE(handle, fsp, data, n, offset);
 
-	DEBUG(10, ("vfs_private = %x\n",
-		   (unsigned int)fsp->fsp_name->st.vfs_private));
-
 	if ((ret != -1) &&
-	    ((fsp->fsp_name->st.vfs_private & GPFS_WINATTR_OFFLINE) != 0)) {
+	    SMB_VFS_IS_OFFLINE(handle->conn, fsp->fsp_name, &fsp->fsp_name->st))
+	{
 		fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
 		notify_fname(handle->conn, NOTIFY_ACTION_MODIFIED,
 			     FILE_NOTIFY_CHANGE_ATTRIBUTES,
@@ -2298,11 +2293,9 @@ static ssize_t vfs_gpfs_pwrite_recv(struct tevent_req *req, int *err)
 	}
 	*err = state->err;
 
-	DEBUG(10, ("vfs_private = %x\n",
-		   (unsigned int)fsp->fsp_name->st.vfs_private));
-
 	if ((state->ret != -1) &&
-	    ((fsp->fsp_name->st.vfs_private & GPFS_WINATTR_OFFLINE) != 0)) {
+	    SMB_VFS_IS_OFFLINE(fsp->conn, fsp->fsp_name, &fsp->fsp_name->st))
+	{
 		fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
 		DEBUG(10, ("sending notify\n"));
 		notify_fname(fsp->conn, NOTIFY_ACTION_MODIFIED,
-- 
1.9.1


From 3e41bfc95b16cb4425add434bf773949d72e5aa8 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 17 Jul 2014 23:35:58 +0200
Subject: [PATCH 3/4] s3:vfs:gpfs: remove all writing uses of
 stat_ex.vfs_private from vfs_gpfs.

Now that the vfs_private cache is never read in vfs_gpfs, there is
no need any more to write it.

With this change, vfs_gpfs does not use vfs_private any more.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/modules/vfs_gpfs.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
index 54cdd32..beec204 100644
--- a/source3/modules/vfs_gpfs.c
+++ b/source3/modules/vfs_gpfs.c
@@ -1590,7 +1590,6 @@ static int vfs_gpfs_stat(struct vfs_handle_struct *handle,
 		smb_fname->st.st_ex_calculated_birthtime = false;
 		smb_fname->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
 		smb_fname->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
-		smb_fname->st.vfs_private = attrs.winAttrs;
 	}
 	return 0;
 }
@@ -1622,7 +1621,6 @@ 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;
 }
@@ -1668,7 +1666,6 @@ static int vfs_gpfs_lstat(struct vfs_handle_struct *handle,
 		smb_fname->st.st_ex_calculated_birthtime = false;
 		smb_fname->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
 		smb_fname->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
-		smb_fname->st.vfs_private = attrs.winAttrs;
 	}
 	return 0;
 }
@@ -2141,7 +2138,6 @@ static ssize_t vfs_gpfs_pread(vfs_handle_struct *handle, files_struct *fsp,
 	if ((ret != -1) &&
 	    SMB_VFS_IS_OFFLINE(handle->conn, fsp->fsp_name, &fsp->fsp_name->st))
 	{
-		fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
 		notify_fname(handle->conn, NOTIFY_ACTION_MODIFIED,
 			     FILE_NOTIFY_CHANGE_ATTRIBUTES,
 			     fsp->fsp_name->base_name);
@@ -2208,7 +2204,6 @@ static ssize_t vfs_gpfs_pread_recv(struct tevent_req *req, int *err)
 	if ((state->ret != -1) &&
 	    SMB_VFS_IS_OFFLINE(fsp->conn, fsp->fsp_name, &fsp->fsp_name->st))
 	{
-		fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
 		DEBUG(10, ("sending notify\n"));
 		notify_fname(fsp->conn, NOTIFY_ACTION_MODIFIED,
 			     FILE_NOTIFY_CHANGE_ATTRIBUTES,
@@ -2228,7 +2223,6 @@ static ssize_t vfs_gpfs_pwrite(vfs_handle_struct *handle, files_struct *fsp,
 	if ((ret != -1) &&
 	    SMB_VFS_IS_OFFLINE(handle->conn, fsp->fsp_name, &fsp->fsp_name->st))
 	{
-		fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
 		notify_fname(handle->conn, NOTIFY_ACTION_MODIFIED,
 			     FILE_NOTIFY_CHANGE_ATTRIBUTES,
 			     fsp->fsp_name->base_name);
@@ -2296,7 +2290,6 @@ static ssize_t vfs_gpfs_pwrite_recv(struct tevent_req *req, int *err)
 	if ((state->ret != -1) &&
 	    SMB_VFS_IS_OFFLINE(fsp->conn, fsp->fsp_name, &fsp->fsp_name->st))
 	{
-		fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
 		DEBUG(10, ("sending notify\n"));
 		notify_fname(fsp->conn, NOTIFY_ACTION_MODIFIED,
 			     FILE_NOTIFY_CHANGE_ATTRIBUTES,
-- 
1.9.1


From 41e0b9cc04dc4631ee95f160f2e3a90f03531be0 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 17 Jul 2014 17:06:32 +0200
Subject: [PATCH 4/4] s3:vfs:gpfs: remove a block and reduce indentation in
 gpfs_is_offline()

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/modules/vfs_gpfs.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
index beec204..ff9f71c 100644
--- a/source3/modules/vfs_gpfs.c
+++ b/source3/modules/vfs_gpfs.c
@@ -1801,6 +1801,7 @@ static bool vfs_gpfs_is_offline(struct vfs_handle_struct *handle,
 	char *path = NULL;
 	NTSTATUS status;
 	struct gpfs_config_data *config;
+	int ret;
 
 	SMB_VFS_HANDLE_GET_DATA(handle, config,
 				struct gpfs_config_data,
@@ -1816,15 +1817,12 @@ static bool vfs_gpfs_is_offline(struct vfs_handle_struct *handle,
 		return -1;
 	}
 
-	{
-		int ret;
-		ret = get_gpfs_winattrs(path, &attrs);
-
-		if (ret == -1) {
-			TALLOC_FREE(path);
-			return false;
-		}
+	ret = get_gpfs_winattrs(path, &attrs);
+	if (ret == -1) {
+		TALLOC_FREE(path);
+		return false;
 	}
+
 	if ((attrs.winAttrs & GPFS_WINATTR_OFFLINE) != 0) {
 		DEBUG(10, ("%s is offline\n", path));
 		TALLOC_FREE(path);
-- 
1.9.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-s3-remove-stat_ex.vfs_private-completely.patch
Type: text/x-diff
Size: 3043 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140718/e408c9f4/attachment.patch>
-------------- 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/20140718/e408c9f4/attachment.pgp>


More information about the samba-technical mailing list