[PATCHES] fix flapping offline flag in gpfs module
Michael Adam
obnox at samba.org
Wed Jul 23 04:46:17 MDT 2014
On 2014-07-22 at 15:18 -0700, Christof Schmitt wrote:
> On Sun, Jul 20, 2014 at 12:23:31PM +0200, Michael Adam wrote:
> > On 2014-07-18 at 13:32 -0700, Christof Schmitt wrote:
> > >
> > > My main concern with patches 2 and 3 would be that this now triggers a
> > > notification after each read or write request. On busy systems, that
> > > will likely affect performance.
> >
> > After each read and write request?
> > I don't quite see that. Only if the file is offline.
>
> Sorry i got that wrong. With the proposed patches, no notification would
> get sent after a read or write. The check would be done after completing
> the read or write and at that point the file is no longer flagged as
> offline.
Exactly. And thanks for notifying me of that!
> > The intent was of course to not modify behaviour,
> > but it is in fact modified by the patches, because
> > the original code was operating on a cache (broken,
> > but a cache anyways), and the new code gets the live
> > data. Hence I need to modify the patch to call
> > was_offline = IS_OFFLINE() before e.g. calling NEXT_PREAD()
> > and then checking was_offline after NEXT_PREAD. etc.
>
> Yes, check the offline flag first. If the read or write succeeded and
> the file was offline, then the offline status must have changed and
> other processes should get notified.
>
> > Will post an updated patchset...
Attached an updated patchset.
The second patch is different in that it adds helper variables
and reads offline status before calling pread/pwrite , and checks
the value after the pread/pwrite calls to decide whether to send
out notifications.
The third patch (removing the setting of the vfs_private
variable) only differs in context.
The three other patches are unchanged, and I have hence added
Volker's review from further up in this thread (including his
typo fixes in the commit msgs - thanks for that!). The two
changed patches need review.
Thanks - Michael
-------------- next part --------------
From 2fa882651f8b593cc9da916a4bfe61457f398960 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/5] 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>
Reviewed-by: Volker Lendecke <vl 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 07f1bff29f31b698c634be039c8cbe763d5b7bce 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/5] 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 implementation of gpfs_is_offline() showed, this cache
does not always carry valid information 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. For the pread and pwrite
calls, we need to call IS_OFFLINE before the actual read
and check afterwards if the file was offline before (as a basis
whether to send notifications).
Signed-off-by: Michael Adam <obnox at samba.org>
---
source3/modules/vfs_gpfs.c | 45 ++++++++++++++++++++++-----------------------
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
index 9fcce78..8049cb7 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;
@@ -2134,14 +2135,14 @@ static ssize_t vfs_gpfs_pread(vfs_handle_struct *handle, files_struct *fsp,
void *data, size_t n, off_t offset)
{
ssize_t ret;
+ bool was_offline;
- ret = SMB_VFS_NEXT_PREAD(handle, fsp, data, n, offset);
+ was_offline = SMB_VFS_IS_OFFLINE(handle->conn, fsp->fsp_name,
+ &fsp->fsp_name->st);
- DEBUG(10, ("vfs_private = %x\n",
- (unsigned int)fsp->fsp_name->st.vfs_private));
+ ret = SMB_VFS_NEXT_PREAD(handle, fsp, data, n, offset);
- if ((ret != -1) &&
- ((fsp->fsp_name->st.vfs_private & GPFS_WINATTR_OFFLINE) != 0)) {
+ if ((ret != -1) && was_offline) {
fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
notify_fname(handle->conn, NOTIFY_ACTION_MODIFIED,
FILE_NOTIFY_CHANGE_ATTRIBUTES,
@@ -2155,6 +2156,7 @@ struct vfs_gpfs_pread_state {
struct files_struct *fsp;
ssize_t ret;
int err;
+ bool was_offline;
};
static void vfs_gpfs_pread_done(struct tevent_req *subreq);
@@ -2173,6 +2175,8 @@ static struct tevent_req *vfs_gpfs_pread_send(struct vfs_handle_struct *handle,
if (req == NULL) {
return NULL;
}
+ state->was_offline = SMB_VFS_IS_OFFLINE(handle->conn, fsp->fsp_name,
+ &fsp->fsp_name->st);
state->fsp = fsp;
subreq = SMB_VFS_NEXT_PREAD_SEND(state, ev, handle, fsp, data,
n, offset);
@@ -2206,11 +2210,7 @@ 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)) {
+ if ((state->ret != -1) && state->was_offline) {
fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
DEBUG(10, ("sending notify\n"));
notify_fname(fsp->conn, NOTIFY_ACTION_MODIFIED,
@@ -2225,14 +2225,14 @@ static ssize_t vfs_gpfs_pwrite(vfs_handle_struct *handle, files_struct *fsp,
const void *data, size_t n, off_t offset)
{
ssize_t ret;
+ bool was_offline;
- ret = SMB_VFS_NEXT_PWRITE(handle, fsp, data, n, offset);
+ was_offline = SMB_VFS_IS_OFFLINE(handle->conn, fsp->fsp_name,
+ &fsp->fsp_name->st);
- DEBUG(10, ("vfs_private = %x\n",
- (unsigned int)fsp->fsp_name->st.vfs_private));
+ ret = SMB_VFS_NEXT_PWRITE(handle, fsp, data, n, offset);
- if ((ret != -1) &&
- ((fsp->fsp_name->st.vfs_private & GPFS_WINATTR_OFFLINE) != 0)) {
+ if ((ret != -1) && was_offline) {
fsp->fsp_name->st.vfs_private &= ~GPFS_WINATTR_OFFLINE;
notify_fname(handle->conn, NOTIFY_ACTION_MODIFIED,
FILE_NOTIFY_CHANGE_ATTRIBUTES,
@@ -2246,6 +2246,7 @@ struct vfs_gpfs_pwrite_state {
struct files_struct *fsp;
ssize_t ret;
int err;
+ bool was_offline;
};
static void vfs_gpfs_pwrite_done(struct tevent_req *subreq);
@@ -2265,6 +2266,8 @@ static struct tevent_req *vfs_gpfs_pwrite_send(
if (req == NULL) {
return NULL;
}
+ state->was_offline = SMB_VFS_IS_OFFLINE(handle->conn, fsp->fsp_name,
+ &fsp->fsp_name->st);
state->fsp = fsp;
subreq = SMB_VFS_NEXT_PWRITE_SEND(state, ev, handle, fsp, data,
n, offset);
@@ -2298,11 +2301,7 @@ 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)) {
+ if ((state->ret != -1) && state->was_offline) {
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 514a3146438d1ead074f15087d4aadd7847f2c38 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/5] 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 8049cb7..073060c 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;
}
@@ -2143,7 +2140,6 @@ static ssize_t vfs_gpfs_pread(vfs_handle_struct *handle, files_struct *fsp,
ret = SMB_VFS_NEXT_PREAD(handle, fsp, data, n, offset);
if ((ret != -1) && was_offline) {
- 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);
@@ -2211,7 +2207,6 @@ static ssize_t vfs_gpfs_pread_recv(struct tevent_req *req, int *err)
*err = state->err;
if ((state->ret != -1) && state->was_offline) {
- 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,
@@ -2233,7 +2228,6 @@ static ssize_t vfs_gpfs_pwrite(vfs_handle_struct *handle, files_struct *fsp,
ret = SMB_VFS_NEXT_PWRITE(handle, fsp, data, n, offset);
if ((ret != -1) && was_offline) {
- 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);
@@ -2302,7 +2296,6 @@ static ssize_t vfs_gpfs_pwrite_recv(struct tevent_req *req, int *err)
*err = state->err;
if ((state->ret != -1) && state->was_offline) {
- 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 4cc1aff616ae0adf8c17f5b0140322ea6e636c7d 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/5] s3:vfs:gpfs: remove a block and reduce indentation in
gpfs_is_offline()
Signed-off-by: Michael Adam <obnox at samba.org>
Reviewed-by: Volker Lendecke <vl 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 073060c..e722a86 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
From ba4d54413a84b3810bc7e2e693f12b1c188f9884 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 17 Jul 2014 17:27:17 +0200
Subject: [PATCH 5/5] s3: remove stat_ex.vfs_private completely
It is not used any more.
Signed-off-by: Michael Adam <obnox at samba.org>
Reviewed-by: Volker Lendecke <vl at samba.org>
---
source3/include/includes.h | 8 --------
source3/librpc/idl/open_files.idl | 1 -
source3/smbd/durable.c | 14 --------------
3 files changed, 23 deletions(-)
diff --git a/source3/include/includes.h b/source3/include/includes.h
index de44fd2..0715608 100644
--- a/source3/include/includes.h
+++ b/source3/include/includes.h
@@ -322,14 +322,6 @@ struct stat_ex {
uint32_t st_ex_flags;
uint32_t st_ex_mask;
-
- /*
- * Add space for VFS internal extensions. The initial user of this
- * would be the onefs modules, passing the snapid from the stat calls
- * to the file_id_create call. Maybe we'll have to expand this later,
- * but the core of Samba should never look at this field.
- */
- uint64_t vfs_private;
};
typedef struct stat_ex SMB_STRUCT_STAT;
diff --git a/source3/librpc/idl/open_files.idl b/source3/librpc/idl/open_files.idl
index 0ebc819..4278301 100644
--- a/source3/librpc/idl/open_files.idl
+++ b/source3/librpc/idl/open_files.idl
@@ -76,7 +76,6 @@ interface open_files
hyper st_ex_blocks;
uint32 st_ex_flags;
uint32 st_ex_mask;
- hyper vfs_private;
} vfs_default_durable_stat;
typedef [public] struct {
diff --git a/source3/smbd/durable.c b/source3/smbd/durable.c
index 0da734e..9489cf1 100644
--- a/source3/smbd/durable.c
+++ b/source3/smbd/durable.c
@@ -121,7 +121,6 @@ NTSTATUS vfs_default_durable_cookie(struct files_struct *fsp,
cookie.stat_info.st_ex_blocks = fsp->fsp_name->st.st_ex_blocks;
cookie.stat_info.st_ex_flags = fsp->fsp_name->st.st_ex_flags;
cookie.stat_info.st_ex_mask = fsp->fsp_name->st.st_ex_mask;
- cookie.stat_info.vfs_private = fsp->fsp_name->st.vfs_private;
ndr_err = ndr_push_struct_blob(cookie_blob, mem_ctx, &cookie,
(ndr_push_flags_fn_t)ndr_push_vfs_default_durable_cookie);
@@ -275,7 +274,6 @@ NTSTATUS vfs_default_durable_disconnect(struct files_struct *fsp,
cookie.stat_info.st_ex_blocks = fsp->fsp_name->st.st_ex_blocks;
cookie.stat_info.st_ex_flags = fsp->fsp_name->st.st_ex_flags;
cookie.stat_info.st_ex_mask = fsp->fsp_name->st.st_ex_mask;
- cookie.stat_info.vfs_private = fsp->fsp_name->st.vfs_private;
ndr_err = ndr_push_struct_blob(&new_cookie_blob, mem_ctx, &cookie,
(ndr_push_flags_fn_t)ndr_push_vfs_default_durable_cookie);
@@ -536,18 +534,6 @@ static bool vfs_default_durable_reconnect_check_stat(
return false;
}
- if (cookie_st->vfs_private != fsp_st->vfs_private) {
- DEBUG(1, ("vfs_default_durable_reconnect (%s): "
- "stat_ex.%s differs: "
- "cookie:%llu != stat:%llu, "
- "denying durable reconnect\n",
- name,
- "vfs_private",
- (unsigned long long)cookie_st->vfs_private,
- (unsigned long long)fsp_st->vfs_private));
- return false;
- }
-
return true;
}
--
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/20140723/96ad17d9/attachment.pgp>
More information about the samba-technical
mailing list