[Samba] stat between reads

[Jeremy Allison]

On Fri, Mar 22, 2013 at 02:07:29PM -0700, Jeremy Allison wrote:
> On Fri, Mar 22, 2013 at 05:24:20PM +0100, Volker Lendecke wrote:
> > If I see it right, we could avoid most of those calls.
> > First, they are only relevant to see whether we can do
> > sendfile. That choice is racy, we first look and have to
> > deal with the mess if we have a short read afterwards
> > anyway.
> > 
> > Jeremy, what do you think? Only do that stat call in the
> > sendfile if-branch, and there only if the read call in
> > question would go beyond what we currently have in
> > st.st_ex_size?
> 
> Yes.... we could certainly make that change. It's not
> relevent in the non-sendfile read path but we'd have to be
> careful about not doing it by checking the existing
> fsp->fsp_name->st.st_ex_size, as I don't think we
> update that on an ftruncate call.
> 
> My worry would be (to one single smbd):
> 
> open 1mb file
> ftruncate to 10k
> readX of 50k at offset 0.
> 
> Unless the ftruncate call updates fsp->fsp_name->st.st_ex_size
> then we'll return 10k of real data + 40k of zeros if
> sendfile is turned on, rather than a correct short read of
> 10k.
> 
> Let me look at the code some and revisit this.

Yep, I was right. This would be a problem (ftruncate
and other file-size changing calls don't automatically
update the st_ex_size on an fsp).

> Moving the fstat and ISREG check to the use_sendfile()
> path is an obviously correct no brainer though.

And here is that patch. Should apply cleanly to
4.0.x and 3.6.x (with a few offsets). I'm testing
here and will propose as an official optimization
if 'make test' passes locally.

Jeremy.
-------------- next part --------------
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 2326015..7ca92ed 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3666,11 +3666,6 @@ static void send_file_readX(connection_struct *conn, struct smb_request *req,
 	struct lock_struct lock;
 	int saved_errno = 0;
 
-	if(fsp_stat(fsp) == -1) {
-		reply_nterror(req, map_nt_error_from_unix(errno));
-		return;
-	}
-
 	init_strict_lock_struct(fsp, (uint64_t)req->smbpid,
 	    (uint64_t)startpos, (uint64_t)smb_maxcnt, READ_LOCK,
 	    &lock);
@@ -3680,16 +3675,6 @@ static void send_file_readX(connection_struct *conn, struct smb_request *req,
 		return;
 	}
 
-	if (!S_ISREG(fsp->fsp_name->st.st_ex_mode) ||
-			(startpos > fsp->fsp_name->st.st_ex_size)
-			|| (smb_maxcnt > (fsp->fsp_name->st.st_ex_size - startpos))) {
-		/*
-		 * We already know that we would do a short read, so don't
-		 * try the sendfile() path.
-		 */
-		goto nosendfile_read;
-	}
-
 	/*
 	 * We can only use sendfile on a non-chained packet
 	 * but we can use on a non-oplocked file. tridge proved this
@@ -3704,6 +3689,21 @@ static void send_file_readX(connection_struct *conn, struct smb_request *req,
 		uint8 headerbuf[smb_size + 12 * 2];
 		DATA_BLOB header;
 
+		if(fsp_stat(fsp) == -1) {
+			reply_nterror(req, map_nt_error_from_unix(errno));
+			return;
+		}
+
+		if (!S_ISREG(fsp->fsp_name->st.st_ex_mode) ||
+				(startpos > fsp->fsp_name->st.st_ex_size)
+				|| (smb_maxcnt > (fsp->fsp_name->st.st_ex_size - startpos))) {
+			/*
+			 * We already know that we would do a short read, so don't
+			 * try the sendfile() path.
+			 */
+			goto nosendfile_read;
+		}
+
 		/*
 		 * Set up the packet header before send. We
 		 * assume here the sendfile will work (get the