[patch 21/40] reiserfs: use generic readdir for operations across all xattrs

[jeffm@suse.com]

[-- Attachment #1: reiserfs-kill-xattr-readdir.diff --]
[-- Type: text/plain, Size: 14989 bytes --]

 The current reiserfs xattr implementation open codes reiserfs_readdir and
 frees the path before calling the filldir function. Typically, the filldir
 function is something that modifies the file system, such as a chown or
 an inode deletion that also require reading of an inode associated with each
 direntry. Since the file system is modified, the path retained becomes
 invalid for the next run. In addition, it runs backwards in attempt to
 minimize activity.

 This is clearly suboptimal from a code cleanliness perspective as well as
 performance-wise.

 This patch implements a generic reiserfs_for_each_xattr that uses the generic
 readdir and a specific filldir routine that simply populates an array of
 dentries and then performs a specific operation on them. When all files have
 been operated on, it then calls the operation on the directory itself.

 The result is a noticable code reduction and better performance.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>

--
 fs/reiserfs/xattr.c |  439 +++++++++++++++-------------------------------------
 1 file changed, 129 insertions(+), 310 deletions(-)

--- a/fs/reiserfs/xattr.c	2007-06-11 14:49:38.000000000 -0400
+++ b/fs/reiserfs/xattr.c	2007-06-11 14:49:38.000000000 -0400
@@ -191,182 +191,6 @@ out:
 	return dentry_open(xafile, NULL, O_RDWR|O_NOATIME);
 }
 
-/*
- * this is very similar to fs/reiserfs/dir.c:reiserfs_readdir, but
- * we need to drop the path before calling the filldir struct.  That
- * would be a big performance hit to the non-xattr case, so I've copied
- * the whole thing for now. --clm
- *
- * the big difference is that I go backwards through the directory,
- * and don't mess with f->f_pos, but the idea is the same.  Do some
- * action on each and every entry in the directory.
- *
- * we're called with i_mutex held, so there are no worries about the directory
- * changing underneath us.
- */
-static int __xattr_readdir(struct file *filp, void *dirent, filldir_t filldir)
-{
-	struct inode *inode = filp->f_path.dentry->d_inode;
-	struct cpu_key pos_key;	/* key of current position in the directory (key of directory entry) */
-	INITIALIZE_PATH(path_to_entry);
-	struct buffer_head *bh;
-	int entry_num;
-	struct item_head *ih, tmp_ih;
-	int search_res;
-	char *local_buf;
-	loff_t next_pos;
-	char small_buf[32];	/* avoid kmalloc if we can */
-	struct reiserfs_de_head *deh;
-	int d_reclen;
-	char *d_name;
-	off_t d_off;
-	ino_t d_ino;
-	struct reiserfs_dir_entry de;
-
-	/* form key for search the next directory entry using f_pos field of
-	   file structure */
-	next_pos = max_reiserfs_offset(inode);
-
-	while (1) {
-	      research:
-		if (next_pos <= DOT_DOT_OFFSET)
-			break;
-		make_cpu_key(&pos_key, inode, next_pos, TYPE_DIRENTRY, 3);
-
-		search_res =
-		    search_by_entry_key(inode->i_sb, &pos_key, &path_to_entry,
-					&de);
-		if (search_res == IO_ERROR) {
-			// FIXME: we could just skip part of directory which could
-			// not be read
-			pathrelse(&path_to_entry);
-			return -EIO;
-		}
-
-		if (search_res == NAME_NOT_FOUND)
-			de.de_entry_num--;
-
-		set_de_name_and_namelen(&de);
-		entry_num = de.de_entry_num;
-		deh = &(de.de_deh[entry_num]);
-
-		bh = de.de_bh;
-		ih = de.de_ih;
-
-		if (!is_direntry_le_ih(ih)) {
-			reiserfs_error(inode->i_sb, "jdm-20000",
-			               "not direntry %h", ih);
-			break;
-		}
-		copy_item_head(&tmp_ih, ih);
-
-		/* we must have found item, that is item of this directory, */
-		RFALSE(COMP_SHORT_KEYS(&(ih->ih_key), &pos_key),
-		       "vs-9000: found item %h does not match to dir we readdir %K",
-		       ih, &pos_key);
-
-		if (deh_offset(deh) <= DOT_DOT_OFFSET) {
-			break;
-		}
-
-		/* look for the previous entry in the directory */
-		next_pos = deh_offset(deh) - 1;
-
-		if (!de_visible(deh))
-			/* it is hidden entry */
-			continue;
-
-		d_reclen = entry_length(bh, ih, entry_num);
-		d_name = B_I_DEH_ENTRY_FILE_NAME(bh, ih, deh);
-		d_off = deh_offset(deh);
-		d_ino = deh_objectid(deh);
-
-		if (!d_name[d_reclen - 1])
-			d_reclen = strlen(d_name);
-
-		if (d_reclen > REISERFS_MAX_NAME(inode->i_sb->s_blocksize)) {
-			/* too big to send back to VFS */
-			continue;
-		}
-
-		/* Ignore the .reiserfs_priv entry */
-		if (reiserfs_xattrs(inode->i_sb) &&
-		    !old_format_only(inode->i_sb) &&
-		    deh_objectid(deh) ==
-		    le32_to_cpu(INODE_PKEY
-				(REISERFS_SB(inode->i_sb)->priv_root->d_inode)->
-				k_objectid))
-			continue;
-
-		if (d_reclen <= 32) {
-			local_buf = small_buf;
-		} else {
-			local_buf = kmalloc(d_reclen, GFP_NOFS);
-			if (!local_buf) {
-				pathrelse(&path_to_entry);
-				return -ENOMEM;
-			}
-			if (item_moved(&tmp_ih, &path_to_entry)) {
-				kfree(local_buf);
-
-				/* sigh, must retry.  Do this same offset again */
-				next_pos = d_off;
-				goto research;
-			}
-		}
-
-		// Note, that we copy name to user space via temporary
-		// buffer (local_buf) because filldir will block if
-		// user space buffer is swapped out. At that time
-		// entry can move to somewhere else
-		memcpy(local_buf, d_name, d_reclen);
-
-		/* the filldir function might need to start transactions,
-		 * or do who knows what.  Release the path now that we've
-		 * copied all the important stuff out of the deh
-		 */
-		pathrelse(&path_to_entry);
-
-		if (filldir(dirent, local_buf, d_reclen, d_off, d_ino,
-			    DT_UNKNOWN) < 0) {
-			if (local_buf != small_buf) {
-				kfree(local_buf);
-			}
-			goto end;
-		}
-		if (local_buf != small_buf) {
-			kfree(local_buf);
-		}
-	}			/* while */
-
-      end:
-	pathrelse(&path_to_entry);
-	return 0;
-}
-
-/*
- * this could be done with dedicated readdir ops for the xattr files,
- * but I want to get something working asap
- * this is stolen from vfs_readdir
- *
- */
-static
-int xattr_readdir(struct file *file, filldir_t filler, void *buf)
-{
-	struct inode *inode = file->f_path.dentry->d_inode;
-	int res = -ENOTDIR;
-	if (!file->f_op || !file->f_op->readdir)
-		goto out;
-	res = -ENOENT;
-	if (!IS_DEADDIR(inode)) {
-		lock_kernel();
-		res = __xattr_readdir(file, buf, filler);
-		unlock_kernel();
-	}
-      out:
-	return res;
-}
-
 /* Internal operations on file data */
 static inline void reiserfs_put_page(struct page *page)
 {
@@ -670,152 +494,83 @@ reiserfs_xattr_get(struct inode *inode, 
 	return err;
 }
 
-/* The following are side effects of other operations that aren't explicitly
- * modifying extended attributes. This includes operations such as permissions
- * or ownership changes, object deletions, etc. */
-
-static int
-reiserfs_delete_xattrs_filler(void *buf, const char *name, int namelen,
-			      loff_t offset, u64 ino, unsigned int d_type)
+#if 0
+static int __restart_transaction_if_needed(struct inode *inode, int chunk)
 {
-	struct dentry *xadir = (struct dentry *)buf;
-	struct dentry *dentry;
-	int err = 0;
+	struct reiserfs_transaction_handle *th = current->journal_info;
+	struct super_block *s = th->t_super;
+	int len = th->t_blocks_allocated;
+	int err;
 
-	dentry = lookup_one_len(name, xadir, namelen);
-	if (IS_ERR(dentry)) {
-		err = PTR_ERR(dentry);
-		goto out;
-	} else if (!dentry->d_inode) {
-		err = -ENODATA;
-		goto out_file;
-	}
+	BUG_ON(!th->t_trans_id);
+	BUG_ON(!th->t_refcount);
 
-	/* Skip directories.. */
-	if (S_ISDIR(dentry->d_inode->i_mode))
-		goto out_file;
-
-	err = vfs_unlink(xadir->d_inode, dentry);
-
-      out_file:
-	dput(dentry);
-
-      out:
-	return err;
-}
-
-/* This is called w/ inode->i_mutex downed */
-int reiserfs_delete_xattrs(struct inode *inode)
-{
-	struct file *fp;
-	struct dentry *dir, *root;
-	int err = 0;
-
-	/* Skip out, an xattr has no xattrs associated with it */
-	if (is_reiserfs_priv_object(inode) ||
-	    get_inode_sd_version(inode) == STAT_DATA_V1 ||
-	    !reiserfs_xattrs(inode->i_sb)) {
+	if (!journal_transaction_should_end(th, len))
 		return 0;
-	}
-	dir = open_xa_dir(inode, XATTR_REPLACE);
-	if (IS_ERR(dir)) {
-		err = PTR_ERR(dir);
-		goto out;
-	} else if (!dir->d_inode) {
-		dput(dir);
-		err = 0;
-		goto out;
-	}
 
-	fp = dentry_open(dir, NULL, O_RDWR|O_NOATIME|O_DIRECTORY);
-	if (IS_ERR(fp)) {
-		err = PTR_ERR(fp);
-		/* dentry_open dputs the dentry if it fails */
-		goto out;
+	/* we cannot restart while nested */
+	if (th->t_refcount > 1) {
+		reiserfs_warning(th->t_super, "jdm-20003", "can't restart nested transaction in %s\n", __FUNCTION__);
+		return 0;
 	}
 
-	mutex_lock_nested(&fp->f_path.dentry->d_inode->i_mutex, I_MUTEX_XATTR);
-	lock_kernel();
-	err = xattr_readdir(fp, reiserfs_delete_xattrs_filler, dir);
-	unlock_kernel();
-	mutex_unlock(&fp->f_path.dentry->d_inode->i_mutex);
-	if (err)
-		goto out_dir;
-
-	/* Leftovers besides . and .. -- that's not good. */
-	if (dir->d_inode->i_nlink <= 2) {
-		struct reiserfs_transaction_handle th;
-		int jbegin_count;
-		int jerr;
-
-		jbegin_count = JOURNAL_PER_BALANCE_CNT * 2 + 2 + 4 *
-		               REISERFS_QUOTA_TRANS_BLOCKS(inode->i_sb);
-		reiserfs_write_lock(inode->i_sb);
-		err = journal_begin (&th, inode->i_sb, jbegin_count);
-		if (err) {
-			reiserfs_write_unlock(inode->i_sb);
-			unlock_kernel();
-			goto out_dir;
-		}
-		root = open_xa_root(inode->i_sb, XATTR_REPLACE);
-		mutex_lock_nested(&root->d_inode->i_mutex, I_MUTEX_XATTR);
-		err = vfs_rmdir(root->d_inode, dir);
-		mutex_unlock(&root->d_inode->i_mutex);
-		jerr = journal_end (&th, inode->i_sb, jbegin_count);
-		reiserfs_write_unlock(inode->i_sb);
-		dput(root);
-		if (!err && jerr)
-			err = jerr;
-	} else {
-		reiserfs_warning(inode->i_sb, "jdm-20004",
-				 "Couldn't remove all entries in directory");
+	err = journal_end(th, s, len);
+	if (!err) {
+		err = journal_begin(th, s, chunk);
+		if (!err)
+			reiserfs_update_inode_transaction(inode);
 	}
 
-      out_dir:
-	fput(fp);
-
-      out:
 	return err;
 }
+#endif
 
-struct reiserfs_chown_buf {
-	struct inode *inode;
+/* The following are side effects of other operations that aren't explicitly
+ * modifying extended attributes. This includes operations such as permissions
+ * or ownership changes, object deletions, etc. */
+struct reiserfs_dentry_buf {
 	struct dentry *xadir;
-	struct iattr *attrs;
+	int count;
+	struct dentry *dentries[8];
 };
 
-/* XXX: If there is a better way to do this, I'd love to hear about it */
 static int
-reiserfs_chown_xattrs_filler(void *buf, const char *name, int namelen,
-			     loff_t offset, u64 ino, unsigned int d_type)
+fill_with_dentries(void *buf, const char *name, int namelen, loff_t offset,
+                   u64 ino, unsigned int d_type)
 {
-	struct reiserfs_chown_buf *chown_buf = (struct reiserfs_chown_buf *)buf;
-	struct dentry *xafile, *xadir = chown_buf->xadir;
-	struct iattr *attrs = chown_buf->attrs;
-	int err = 0;
+	struct reiserfs_dentry_buf *dbuf = buf;
+	struct dentry *dentry;
+
+	if (dbuf->count == ARRAY_SIZE(dbuf->dentries))
+		return -ENOSPC;
+
+	if (name[0] == '.' && (name[1] == '\0' || (name[1] == '.' && name[2] == '\0')))
+		return 0;
 
-	xafile = lookup_one_len(name, xadir, namelen);
-	if (IS_ERR(xafile))
-		return PTR_ERR(xafile);
-	else if (!xafile->d_inode) {
-		dput(xafile);
+	dentry = lookup_one_len(name, dbuf->xadir, namelen);
+	if (IS_ERR(dentry)) {
+		return PTR_ERR(dentry);
+	} else if (!dentry->d_inode) {
+		/* This should never happen */
+		dput(dentry);
 		return -ENODATA;
 	}
 
-	if (!S_ISDIR(xafile->d_inode->i_mode))
-		err = notify_change(xafile, attrs);
-	dput(xafile);
-
-	return err;
+	dbuf->dentries[dbuf->count++] = dentry;
+	return 0;
 }
 
-int reiserfs_chown_xattrs(struct inode *inode, struct iattr *attrs)
+typedef int(*xattr_action)(struct dentry *dentry, void *data);
+
+static int reiserfs_for_each_xattr(struct inode *inode, xattr_action action,
+                                   void *data)
 {
 	struct file *fp;
 	struct dentry *dir;
 	int err = 0;
-	struct reiserfs_chown_buf buf;
-	unsigned int ia_valid = attrs->ia_valid;
+	struct reiserfs_dentry_buf buf = {
+		.count = 0,
+	};
 
 	/* Skip out, an xattr has no xattrs associated with it */
 	if (is_reiserfs_priv_object(inode) ||
@@ -841,31 +596,95 @@ int reiserfs_chown_xattrs(struct inode *
 	}
 
 	mutex_lock_nested(&fp->f_path.dentry->d_inode->i_mutex, I_MUTEX_XATTR);
-	lock_kernel();
-
-	attrs->ia_valid &= (ATTR_UID | ATTR_GID | ATTR_CTIME);
 	buf.xadir = dir;
-	buf.attrs = attrs;
-	buf.inode = inode;
 
-	err = xattr_readdir(fp, reiserfs_chown_xattrs_filler, &buf);
-	if (err) {
-		unlock_kernel();
-		goto out_dir;
-	}
+	err = fp->f_op->readdir(fp, &buf, fill_with_dentries);
+	while ((err == 0 || err == -ENOSPC) && buf.count) {
+		int i;
+		err = 0;
 
-	err = notify_change(dir, attrs);
-	unlock_kernel();
+		for (i = 0; i < buf.count && buf.dentries[i]; i++) {
+			int lerr = 0;
+			struct dentry *dentry = buf.dentries[i];
+
+			if (err == 0 && !S_ISDIR(dentry->d_inode->i_mode))
+				lerr = action(dentry, data);
+
+			dput(dentry);
+			buf.dentries[i] = NULL;
+
+			if (lerr)
+				err = lerr;
+		}
+		buf.count = 0;
+		if (err == 0)
+			err = fp->f_op->readdir(fp, &buf, fill_with_dentries);
+	}
 
-      out_dir:
 	mutex_unlock(&fp->f_path.dentry->d_inode->i_mutex);
+
+	if (err == 0)
+		err = action(dir, data);
+
 	fput(fp);
 
       out:
-	attrs->ia_valid = ia_valid;
 	return err;
 }
 
+static int delete_one_xattr(struct dentry *dentry, void *data)
+{
+	struct inode *dir = dentry->d_parent->d_inode;
+
+	/* This is the xattr dir, handle specially. */
+	if (S_ISDIR(dentry->d_inode->i_mode)) {
+		int err;
+		struct reiserfs_transaction_handle th;
+		int nblocks = JOURNAL_PER_BALANCE_CNT * 2 + 2 +
+		              4 * REISERFS_QUOTA_TRANS_BLOCKS(dir->i_sb);
+
+		BUG_ON(dentry == dentry->d_parent);
+		if (dir == dentry->d_inode) {
+			printk ("dir = %s\n", dentry->d_parent->d_name.name);
+			printk ("dentry = %s\n", dentry->d_name.name);
+			return -EBUSY;
+		}
+
+		/* reiserfs_rmdir is going to start a transaction itself,
+		 * but we need to start it outside of locking the xattr
+		 * root to preserve lock ordering. */
+		err = journal_begin(&th, dir->i_sb, nblocks);
+		if (err == 0) {
+			int err2;
+			mutex_lock_nested(&dir->i_mutex, I_MUTEX_XATTR);
+			err = vfs_rmdir(dir, dentry);
+			err2 = journal_end(&th, dir->i_sb, nblocks);
+			mutex_unlock(&dir->i_mutex);
+			if (err2)
+				err = err2;
+		}
+		return err;
+	}
+
+	return vfs_unlink(dir, dentry);
+}
+
+static int chown_one_xattr(struct dentry *dentry, void *data)
+{
+	struct iattr *attrs = data;
+	return notify_change(dentry, attrs);
+}
+
+int reiserfs_delete_xattrs(struct inode *inode)
+{
+	return reiserfs_for_each_xattr(inode, delete_one_xattr, NULL);
+}
+
+int reiserfs_chown_xattrs(struct inode *inode, struct iattr *attrs)
+{
+	return reiserfs_for_each_xattr(inode, chown_one_xattr, attrs);
+}
+
 /* Actual operations that are exported to VFS-land */
 
 /*
@@ -996,7 +815,7 @@ ssize_t reiserfs_listxattr(struct dentry
 		goto out;
 	}
 
-	err = xattr_readdir(fp, listxattr_filler, &buf);
+	err = fp->f_op->readdir(fp, &buf, listxattr_filler);
 	if (err)
 		goto out_dir;
 

-- 
Jeff Mahoney
SUSE Labs