Re: Q: NFSD readdir in linux-2.6.28

[David Woodhouse <dwmw2@infradead.org>]

On Fri, 2009-04-17 at 23:53 +0100, Al Viro wrote:
> On Fri, Apr 17, 2009 at 11:43:50PM +0100, Al Viro wrote:
> > On Fri, Apr 17, 2009 at 11:17:00PM +0100, David Woodhouse wrote:
> > > It sounds like the better answer is to just make sure i_mutex is held
> > > when nfsd_buffered_readdir() calls back into the provided filldir
> > > function (we could do it in the various filldir functions themselves,
> > > _if_ they call lookup_one_len(), but I think I prefer it this way --
> > > it's simpler). Patch below for comment.
> > 
> > Umm...  I can live with that, assuming that we don't have callbacks
> > that take i_mutex themselves.  AFAICS, everything we call there is
> > either obviously not touching i_mutex or is already called while we
> > hold i_mutex elsewhere, but I'd appreciate if somebody actually
> > tested that sucker for different versions of protocol...
> 
> BTW, why mess with taking i_mutex inside the inner loop and not
> immediately around it?

Or, to phrase my last response slightly differently... because I don't
like either of these two versions very much...



diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index ab93fcf..0523e9f 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1885,9 +1885,10 @@ static int nfsd_buffered_filldir(void *__buf, const char *name, int namlen,
 	return 0;
 }
 
-static int nfsd_buffered_readdir(struct file *file, filldir_t func,
-				 struct readdir_cd *cdp, loff_t *offsetp)
+static __be32 nfsd_buffered_readdir(struct file *file, filldir_t func,
+				    struct readdir_cd *cdp, loff_t *offsetp)
 {
+	struct inode *dir_inode = file->f_path.dentry->d_inode;
 	struct readdir_data buf;
 	struct buffered_dirent *de;
 	int host_err;
@@ -1896,7 +1897,7 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
 
 	buf.dirent = (void *)__get_free_page(GFP_KERNEL);
 	if (!buf.dirent)
-		return -ENOMEM;
+		return nfserrno(-ENOMEM);
 
 	offset = *offsetp;
 
@@ -1912,23 +1913,37 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
 			host_err = 0;
 
 		if (host_err < 0)
-			break;
+			goto done;
 
 		size = buf.used;
 
 		if (!size)
-			break;
+			goto done;
 
 		de = (struct buffered_dirent *)buf.dirent;
+
+		host_err = mutex_lock_killable(&dir_inode->i_mutex);
+		if (host_err)
+			goto done;
+
 		while (size > 0) {
+
+			int finished;
+
 			offset = de->offset;
 
+			/*
+			 * Various filldir functions may end up calling back
+			 * into lookup_one_len() and the file system's 
+			 * ->lookup() method. These expect i_mutex to be held.
+			 */
+
 			if (func(cdp, de->name, de->namlen, de->offset,
 				 de->ino, de->d_type))
-				goto done;
-
+				goto done_unlock;
+			    
 			if (cdp->err != nfs_ok)
-				goto done;
+				goto done_unlock;
 
 			reclen = ALIGN(sizeof(*de) + de->namlen,
 				       sizeof(u64));
@@ -1937,6 +1952,8 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
 		}
 		offset = vfs_llseek(file, 0, SEEK_CUR);
 	}
+ done_unlock:
+	mutex_unlock(&dir_inode->i_mutex);
 
  done:
 	free_page((unsigned long)(buf.dirent));



====================================================================

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index ab93fcf..915a47c 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1885,8 +1885,8 @@ static int nfsd_buffered_filldir(void *__buf, const char *name, int namlen,
 	return 0;
 }
 
-static int nfsd_buffered_readdir(struct file *file, filldir_t func,
-				 struct readdir_cd *cdp, loff_t *offsetp)
+static __be32 nfsd_buffered_readdir(struct file *file, filldir_t func,
+				    struct readdir_cd *cdp, loff_t *offsetp)
 {
 	struct readdir_data buf;
 	struct buffered_dirent *de;
@@ -1896,11 +1896,12 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
 
 	buf.dirent = (void *)__get_free_page(GFP_KERNEL);
 	if (!buf.dirent)
-		return -ENOMEM;
+		return nfserrno(-ENOMEM);
 
 	offset = *offsetp;
 
 	while (1) {
+		struct inode *dir_inode = file->f_path.dentry->d_inode;
 		unsigned int reclen;
 
 		cdp->err = nfserr_eof; /* will be cleared on successful read */
@@ -1920,24 +1921,42 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
 			break;
 
 		de = (struct buffered_dirent *)buf.dirent;
+
+		host_err = mutex_lock_killable(&dir_inode->i_mutex);
+		if (host_err)
+			break;
+
 		while (size > 0) {
+			int finished;
+
 			offset = de->offset;
 
+			/*
+			 * Various filldir functions may end up calling back
+			 * into lookup_one_len() and the file system's 
+			 * ->lookup() method. These expect i_mutex to be held.
+			 */
+
 			if (func(cdp, de->name, de->namlen, de->offset,
 				 de->ino, de->d_type))
-				goto done;
-
+				goto done_unlock;
+			    
 			if (cdp->err != nfs_ok)
-				goto done;
+				goto done_unlock;
 
 			reclen = ALIGN(sizeof(*de) + de->namlen,
 				       sizeof(u64));
 			size -= reclen;
 			de = (struct buffered_dirent *)((char *)de + reclen);
 		}
+		mutex_unlock(&dir_inode->i_mutex);
 		offset = vfs_llseek(file, 0, SEEK_CUR);
-	}
+		continue;
 
+	done_unlock:
+		mutex_unlock(&dir_inode->i_mutex);
+		break;
+	}
  done:
 	free_page((unsigned long)(buf.dirent));
 

-- 
dwmw2