[PATCHES] fix flapping offline flag in gpfs module
Volker Lendecke
Volker.Lendecke at SerNet.DE
Fri Jul 18 03:22:28 MDT 2014
On Fri, Jul 18, 2014 at 12:19:00AM +0200, Michael Adam wrote:
> 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.
Find the patches with my R-B and two typo fixes in the commit messages
attached. This also contains the vfs_private removal, which is a good
thing from my point of view. I think vfs_private is dangerous at best,
there is no protection at all against conflicting use in multiple
modules. If we really need such a thing, I think we would have to add
something like the fsp extensions. But I would really like to know why
vfs_private is really needed.
When looking at this area, I'd much rather see winattrs officially added
to stat_ex, potentially with an additional flag that the winattrs are
not valid here. There's at least GPFS that has special support for the
4 windows bits, and I could see carving out 4 bits from an inode to be
a very worthwhile optimization for specialized file systems. An xattr
potentially has much more overhead. And as we can then store the OFFLINE
flag in the winattrs, we'd have a much cleaner way to do this.
When adding a validity-bit to the winattrs, this would then
also lead to one further step: statlite, as referenced in
http://www.mcs.anl.gov/uploads/cels/papers/TM-302-FINAL.pdf as
an important performance optimization in the networked file system
world. It can be very difficult to correctly query the mtime and file
size, and in particular in unix_convert() Samba does not need that
information at all. Also, if a file is heavily accessed concurrently,
Samba typically maintains its own write timestamp. We don't have to ask
the file system for the correct mtime in a SMB2_FIND. But that's much
further down the road.
> 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.
+1.
Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From f71bc8a416cc0e1fa77c3dd73903dff11bad39a3 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.7.9.5
From 25b535685b9f73062dd562748d75c3d646847e09 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 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>
Reviewed-by: Volker Lendecke <vl 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.7.9.5
From 717ba76c252b88b8fec9a5139ccab15ee2a5d9db 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>
Reviewed-by: Volker Lendecke <vl 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.7.9.5
From e4b0c37fc9dc5ff8a3fa733459ecd77bc6ae9c1f 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 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.7.9.5
From 78117f5b799f0942ade330b490ab07ecc64da7fb 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.7.9.5
More information about the samba-technical
mailing list