[PATCH] vfs_gpfs: move btime stuff from stat to get_dos_attrs
Christof Schmitt
cs at samba.org
Thu Dec 15 18:47:08 UTC 2016
On Thu, Dec 15, 2016 at 06:21:17PM +0100, Ralph Böhme wrote:
> Hi!
>
> Attached is a patch that addresses a performance problem in vfs_gpfs:
>
> perf report found that in a kernel copy workload smbd was spending
> considerable CPU time in vfs_gpfs_(f|l)stat -> gpfs_get_winattrs.
>
> Most of the time the VFS stat caller is not interested in the btime. The
> SMB frontend processing around btime is designed to fetch btime together
> with DOS attributes via dos_mode() in all places that need these
> attributes. That's the way it is implemented in the default VFS module
> and that's what vfs_gpfs now does as well for performance reasons.
>
> Please review and push if ok. Thanks!
I like the approach of aligning with the common code and removing some
complexity from vfs_gpfs.
To compile the module, these changes were necessary on top of your patch:
diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
index 32dc527..f7434c9 100644
--- a/source3/modules/vfs_gpfs.c
+++ b/source3/modules/vfs_gpfs.c
@@ -1565,9 +1565,9 @@ static NTSTATUS vfs_gpfs_get_dos_attributes(struct vfs_handle_struct *handle,
}
*dosmode |= vfs_gpfs_winattrs_to_dosmode(attrs.winAttrs);
- fsp->fsp_name->st.st_ex_calculated_birthtime = false;
- fsp->fsp_name->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
- fsp->fsp_name->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
+ 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;
return NT_STATUS_OK;
}
@@ -1600,9 +1600,9 @@ static NTSTATUS vfs_gpfs_fget_dos_attributes(struct vfs_handle_struct *handle,
}
*dosmode |= vfs_gpfs_winattrs_to_dosmode(attrs.winAttrs);
- 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;
+ fsp->fsp_name->st.st_ex_calculated_birthtime = false;
+ fsp->fsp_name->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
+ fsp->fsp_name->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
return NT_STATUS_OK;
}
Did you post a previous version of the patch by chance instead of the final one?
>
> Cheerio!
> -slow
> From 8cdef4977ab2f85d3dddb3974e34fd0cc9e391c8 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 15 Dec 2016 07:09:58 +0100
> Subject: [PATCH 1/3] vfs_gpfs: update btime in vfs_gpfs_(f)get_dos_attributes
>
> This paves the way for removing btime updates from the stat VFS
> functions.
>
> This way we behave like the default VFS module where DOS attributes and
> btime are fetched from the same backing store and the frontend is
> designed around using dos_mode() -> SMB_VFS_GET_ATTRIBUTES to update
> both attributes as necessary in the SMB processing.
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/modules/vfs_gpfs.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> index f49ef0d..da7ec7a 100644
> --- a/source3/modules/vfs_gpfs.c
> +++ b/source3/modules/vfs_gpfs.c
> @@ -1596,6 +1596,9 @@ static NTSTATUS vfs_gpfs_get_dos_attributes(struct vfs_handle_struct *handle,
> }
>
> *dosmode |= vfs_gpfs_winattrs_to_dosmode(attrs.winAttrs);
> + 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;
>
> return NT_STATUS_OK;
> }
> @@ -1628,6 +1631,9 @@ static NTSTATUS vfs_gpfs_fget_dos_attributes(struct vfs_handle_struct *handle,
> }
>
> *dosmode |= vfs_gpfs_winattrs_to_dosmode(attrs.winAttrs);
> + fsp->fsp_name->st.st_ex_calculated_birthtime = false;
> + fsp->fsp_name->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
> + fsp->fsp_name->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
>
> return NT_STATUS_OK;
> }
> --
> 2.7.4
>
>
> From ac7d5f1d39d0a73590765e872f50e29215004c5a Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Mon, 28 Nov 2016 12:22:04 +0100
> Subject: [PATCH 2/3] vfs_gpfs: remove updating btime from stat VFS calls
>
> This is now handled by the vfs_gpfs_(f)get_dos_attributes. Getting rid
> of this in the stat VFS functions is a huge performance saver. perf
> report found that in a kernel copy workload smbd was spending
> considerable CPU time in vfs_gpfs_(f|l)stat -> gpfs_get_winattrs.
>
> Most of the time the VFS stat caller is not interested in the btime. The
> SMB frontend processing around btime is designed to fetch btime together
> with DOS attributes via dos_mode() in all places that need these
> attributes. That's the way it is implemented in the default VFS module
> and that's what vfs_gpfs now does as well for performance reasons.
>
> This makes vfs_gpfs_fstat a null op and I'm therefor removing it.
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/modules/vfs_gpfs.c | 82 ++--------------------------------------------
> 1 file changed, 2 insertions(+), 80 deletions(-)
>
> diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> index da7ec7a..fedb5dd 100644
> --- a/source3/modules/vfs_gpfs.c
> +++ b/source3/modules/vfs_gpfs.c
> @@ -1749,9 +1749,6 @@ static int stat_with_capability(struct vfs_handle_struct *handle,
> static int vfs_gpfs_stat(struct vfs_handle_struct *handle,
> struct smb_filename *smb_fname)
> {
> - struct gpfs_winattr attrs;
> - char *fname = NULL;
> - NTSTATUS status;
> int ret;
> struct gpfs_config_data *config;
>
> @@ -1767,66 +1764,12 @@ static int vfs_gpfs_stat(struct vfs_handle_struct *handle,
> ret = stat_with_capability(handle, smb_fname, 0);
> }
> #endif
> - if (ret == -1) {
> - return -1;
> - }
> -
> - if (!config->winattr) {
> - return 0;
> - }
> -
> - status = get_full_smb_filename(talloc_tos(), smb_fname, &fname);
> - if (!NT_STATUS_IS_OK(status)) {
> - errno = map_errno_from_nt_status(status);
> - return -1;
> - }
> - ret = gpfswrap_get_winattrs_path(discard_const_p(char, fname), &attrs);
> - TALLOC_FREE(fname);
> - if (ret == 0) {
> - 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;
> - }
> - return 0;
> -}
> -
> -static int vfs_gpfs_fstat(struct vfs_handle_struct *handle,
> - struct files_struct *fsp, SMB_STRUCT_STAT *sbuf)
> -{
> - struct gpfs_winattr attrs;
> - int ret;
> - struct gpfs_config_data *config;
> -
> - SMB_VFS_HANDLE_GET_DATA(handle, config,
> - struct gpfs_config_data,
> - return -1);
> -
> - ret = SMB_VFS_NEXT_FSTAT(handle, fsp, sbuf);
> - if (ret == -1) {
> - return -1;
> - }
> - if ((fsp->fh == NULL) || (fsp->fh->fd == -1)) {
> - return 0;
> - }
> - if (!config->winattr) {
> - return 0;
> - }
> -
> - ret = gpfswrap_get_winattrs(fsp->fh->fd, &attrs);
> - if (ret == 0) {
> - 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;
> - }
> - return 0;
> + return ret;
> }
>
> static int vfs_gpfs_lstat(struct vfs_handle_struct *handle,
> struct smb_filename *smb_fname)
> {
> - struct gpfs_winattr attrs;
> - char *path = NULL;
> - NTSTATUS status;
> int ret;
> struct gpfs_config_data *config;
>
> @@ -1843,27 +1786,7 @@ static int vfs_gpfs_lstat(struct vfs_handle_struct *handle,
> AT_SYMLINK_NOFOLLOW);
> }
> #endif
> -
> - if (ret == -1) {
> - return -1;
> - }
> - if (!config->winattr) {
> - return 0;
> - }
> -
> - status = get_full_smb_filename(talloc_tos(), smb_fname, &path);
> - if (!NT_STATUS_IS_OK(status)) {
> - errno = map_errno_from_nt_status(status);
> - return -1;
> - }
> - ret = gpfswrap_get_winattrs_path(discard_const_p(char, path), &attrs);
> - TALLOC_FREE(path);
> - if (ret == 0) {
> - 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;
> - }
> - return 0;
> + return ret;
> }
>
> static void timespec_to_gpfs_time(struct timespec ts, gpfs_timestruc_t *gt,
> @@ -2611,7 +2534,6 @@ static struct vfs_fn_pointers vfs_gpfs_fns = {
> .fchmod_fn = vfs_gpfs_fchmod,
> .close_fn = vfs_gpfs_close,
> .stat_fn = vfs_gpfs_stat,
> - .fstat_fn = vfs_gpfs_fstat,
> .lstat_fn = vfs_gpfs_lstat,
> .ntimes_fn = vfs_gpfs_ntimes,
> .aio_force_fn = vfs_gpfs_aio_force,
> --
> 2.7.4
>
>
> From 8902d885db3e7ce83e41cb129037e1bc1eecb0ca Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 15 Dec 2016 18:10:22 +0100
> Subject: [PATCH 3/3] vfs_gpfs: simplify stat_with_capability() ifdef
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/modules/vfs_gpfs.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> index fedb5dd..768bf63 100644
> --- a/source3/modules/vfs_gpfs.c
> +++ b/source3/modules/vfs_gpfs.c
> @@ -1706,10 +1706,10 @@ static NTSTATUS vfs_gpfs_fset_dos_attributes(struct vfs_handle_struct *handle,
> return NT_STATUS_OK;
> }
>
> -#if defined(HAVE_FSTATAT)
> static int stat_with_capability(struct vfs_handle_struct *handle,
> struct smb_filename *smb_fname, int flag)
> {
> +#if defined(HAVE_FSTATAT)
> int fd = -1;
> bool b;
> char *dir_name;
> @@ -1743,8 +1743,10 @@ static int stat_with_capability(struct vfs_handle_struct *handle,
> }
>
> return ret;
> -}
> +#else
> + return -1;
> #endif
> +}
My preference here would be avoiding ifdefs in the middle of the code and
change that to:
#if defined(HAVE_FSTATAT)
static int stat_with_capability(struct vfs_handle_struct *handle,
struct smb_filename *smb_fname, int flag)
{
...
}
#else
static int stat_with_capability(struct vfs_handle_struct *handle,
struct smb_filename *smb_fname, int flag)
{
return -1;
}
#endif
>
> static int vfs_gpfs_stat(struct vfs_handle_struct *handle,
> struct smb_filename *smb_fname)
> @@ -1757,13 +1759,11 @@ static int vfs_gpfs_stat(struct vfs_handle_struct *handle,
> return -1);
>
> ret = SMB_VFS_NEXT_STAT(handle, smb_fname);
> -#if defined(HAVE_FSTATAT)
> if (ret == -1 && errno == EACCES) {
> DEBUG(10, ("Trying stat with capability for %s\n",
> smb_fname->base_name));
> ret = stat_with_capability(handle, smb_fname, 0);
> }
> -#endif
> return ret;
> }
>
> @@ -1778,14 +1778,12 @@ static int vfs_gpfs_lstat(struct vfs_handle_struct *handle,
> return -1);
>
> ret = SMB_VFS_NEXT_LSTAT(handle, smb_fname);
> -#if defined(HAVE_FSTATAT)
> if (ret == -1 && errno == EACCES) {
> DEBUG(10, ("Trying lstat with capability for %s\n",
> smb_fname->base_name));
> ret = stat_with_capability(handle, smb_fname,
> AT_SYMLINK_NOFOLLOW);
> }
> -#endif
> return ret;
> }
>
> --
> 2.7.4
>
More information about the samba-technical
mailing list