[PATCHES] fix flapping offline flag in gpfs module
Christof Schmitt
cs at samba.org
Wed Jul 23 16:16:02 MDT 2014
On Wed, Jul 23, 2014 at 12:46:17PM +0200, Michael Adam wrote:
> 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, the patches look good.
Reviewed-by: Christof Schmitt <cs at samba.org>
> Thanks - Michael
> 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
>
More information about the samba-technical
mailing list