unix_convert() and amount of stat() calls

[Jeremy Allison]

On Wed, Sep 08, 2010 at 02:58:10PM -0700, Jeremy Allison wrote:
> On Tue, Sep 07, 2010 at 09:40:17PM +0200, Stefan (metze) Metzmacher wrote:
> > Hi Jeremy,
> > 
> > what's the reason that we loop over the components
> > from the root to the leaf, when checking them?
> > 
> > Can't we start with the last component?
> > At least if it's related to a wildcard search.
> > 
> > Maybe we only need to check the last 2 components,
> > unless the name is mangled?
> > 
> > The problem I want to solve in this branch:
> > http://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/samba-3.4.7-ctdb-10
> > is this:
> > 
> > FindFirst with 'path\to\some\dir\with\files\*'
> > 
> > triggers the following stat calls
> > 
> > path\to\some\dir\with\files\* => ENOENT (this should be already solved
> > with my patches)
> > path\
> > path\to\
> > path\to\some\
> > path\to\some\dir\
> > path\to\some\dir\with\
> > path\to\some\dir\with\files\
> > path\to\some\dir\with\files\* => ENOENT
> 
> Ok, I have a patch for this. Give me a min and I'll post it.

Ok - here's the patch that should fix it. It passes both
RAW-CHKPATH and RAW-SEARCH. It adds an optimization into
the wildcard lookup code such that it checks if the wildcard
is in the last component of the lookup, and if so tries to
directly do a stat on the parent directory. If this succeeds
it short-circuits the pathname walk and starts the lookup at
the last component.

This means an incoming pathname of :

FindFirst with 'path\to\some\dir\with\files\*'

should now do the following stat calls (presuming
the directory exists of course):

path\to\some\dir\with\files\* => ENOENT
path\to\some\dir\with\files = OK

It doesn't currently add "path\to\some\dir\with\files"
into the stat cache (although that could be added) as
I'd have to alloc and truncate the incoming path at
the same position as the parent directory length, which
might be a little tricky. Anyway that's not neccessary
for correctness.

I'm going to do some more stream testing, and then commit
if it looks good. Metze (or Volker) please comment.

Jeremy
-------------- next part --------------
diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
index 8f9b855..d847e0f 100644
--- a/source3/smbd/filename.c
+++ b/source3/smbd/filename.c
@@ -401,7 +401,28 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
 						status = NT_STATUS_OBJECT_PATH_NOT_FOUND;
 						goto fail;
 					}
+				} else if (ret == 0) {
+					/*
+					 * stat() or lstat() of the parent dir
+					 * succeeded. So start the walk
+					 * at this point.
+					 */
+					status = check_for_dot_component(&parent_fname);
+					if (!NT_STATUS_IS_OK(status)) {
+						goto fail;
+					}
+
+					start = smb_fname->base_name +
+						strlen(parent_fname.base_name);
+
+					DEBUG(5,("unix_convert optimize1: name "
+						"= %s, dirpath = %s, "
+						"start = %s\n",
+						smb_fname->base_name,
+						dirpath,
+						start));
 				}
+
 				/*
 				 * Missing last component is ok - new file.
 				 * Also deal with permission denied elsewhere.
@@ -410,6 +431,49 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
 				goto done;
 			}
 		}
+	} else {
+		/*
+		 * We have a wildcard in the pathname.
+		 *
+		 * Optimization for common case where the wildcard
+		 * is in the last component and the client already
+		 * sent the correct case.
+		 */
+		struct smb_filename parent_fname;
+		ZERO_STRUCT(parent_fname);
+		if (!parent_dirname(ctx, smb_fname->base_name,
+					&parent_fname.base_name,
+					NULL)) {
+			status = NT_STATUS_NO_MEMORY;
+			goto fail;
+		}
+		if (!ms_has_wild(parent_fname.base_name)) {
+			/*
+			 * Wildcard isn't in the parent, i.e.
+			 * it must be in the last component.
+			 */
+			if (posix_pathnames) {
+				ret = SMB_VFS_LSTAT(conn, &parent_fname);
+			} else {
+				ret = SMB_VFS_STAT(conn, &parent_fname);
+			}
+			if (ret == 0) {
+				status = check_for_dot_component(&parent_fname);
+				if (!NT_STATUS_IS_OK(status)) {
+					goto fail;
+				}
+
+				start = smb_fname->base_name +
+					strlen(parent_fname.base_name);
+
+				DEBUG(5,("unix_convert optimize2: name "
+					"= %s, dirpath = %s, "
+					"start = %s\n",
+					smb_fname->base_name,
+					dirpath,
+					start));
+			}
+		}
 	}
 
 	/*
@@ -480,6 +544,12 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
 			goto fail;
 		}
 
+		/* Skip the stat call if it's a wildcard end. */
+		if (name_has_wildcard) {
+			DEBUG(5,("Wildcard %s\n",start));
+			goto done;
+		}
+
 		/*
 		 * Check if the name exists up to this point.
 		 */