[PATCH] vfs_gpfs: move btime stuff from stat to get_dos_attrs
Ralph Böhme
slow at samba.org
Thu Dec 15 19:14:57 UTC 2016
On Thu, Dec 15, 2016 at 11:47:08AM -0700, Christof Schmitt wrote:
> 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?
huh, I'm puzzled. That's exactly what my patch 1/3 adds in the first place:
---8<---
--- 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;
}
--
---8<---
It's using struct smb_filename in the path based vfs_gpfs_get_dos_attributes()
and files_struct in the handle based vfs_gpfs_fget_dos_attributes().
How are you applying the patch and are you sure you're applying on top of
master?
*scratches head*
> > 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
>
The code bases uses both forms, but imo shorter wins. :) Cf vfs_default.c, it
uses the shorter form as well. But this commit is just a cleanup, so we can just
drop it as well.
Cheerio!
-slow
More information about the samba-technical
mailing list