[PATCH 1/2] merge xfs_rmdir into xfs_remove

[PATCH 1/2] merge xfs_rmdir into xfs_remove

[Christoph Hellwig]

xfs_remove and xfs_rmdir are almost the same with a little more work
performed in xfs_rmdir due to the . and .. entries.  This patch merges
xfs_rmdir into xfs_remove and performs these actions conditionally.

Also clean up the error handling which was a nightmare in both versions
before.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-05-01 22:56:57.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-05-02 08:30:24.000000000 +0200
@@ -2135,13 +2135,6 @@ again:
 #endif
 }
 
-#ifdef	DEBUG
-#define	REMOVE_DEBUG_TRACE(x)	{remove_which_error_return = (x);}
-int remove_which_error_return = 0;
-#else /* ! DEBUG */
-#define	REMOVE_DEBUG_TRACE(x)
-#endif	/* ! DEBUG */
-
 int
 xfs_remove(
 	xfs_inode_t             *dp,
@@ -2150,6 +2143,7 @@ xfs_remove(
 {
 	xfs_mount_t		*mp = dp->i_mount;
 	xfs_trans_t             *tp = NULL;
+	int			is_dir = S_ISDIR(ip->i_d.di_mode);
 	int                     error = 0;
 	xfs_bmap_free_t         free_list;
 	xfs_fsblock_t           first_block;
@@ -2157,8 +2151,11 @@ xfs_remove(
 	int			committed;
 	int			link_zero;
 	uint			resblks;
+	uint			trans;
+	uint			log_count;
 
 	xfs_itrace_entry(dp);
+	xfs_itrace_entry(ip);
 
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
@@ -2171,19 +2168,25 @@ xfs_remove(
 			return error;
 	}
 
-	xfs_itrace_entry(ip);
-	xfs_itrace_ref(ip);
-
 	error = XFS_QM_DQATTACH(mp, dp, 0);
-	if (!error)
-		error = XFS_QM_DQATTACH(mp, ip, 0);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
+	if (error)
+		goto std_return;
+
+	error = XFS_QM_DQATTACH(mp, ip, 0);
+	if (error)
 		goto std_return;
+
+	if (is_dir) {
+		trans = XFS_TRANS_RMDIR;
+		log_count = XFS_DEFAULT_LOG_COUNT;
+	} else {
+		trans = XFS_TRANS_REMOVE;
+		log_count = XFS_REMOVE_LOG_COUNT;
 	}
 
-	tp = xfs_trans_alloc(mp, XFS_TRANS_REMOVE);
+	tp = xfs_trans_alloc(mp, trans);
 	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
+
 	/*
 	 * We try to get the real space reservation first,
 	 * allowing for directory btree deletion(s) implying
@@ -2195,25 +2198,21 @@ xfs_remove(
 	 */
 	resblks = XFS_REMOVE_SPACE_RES(mp);
 	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
-			XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
+				  XFS_TRANS_PERM_LOG_RES, log_count);
 	if (error == ENOSPC) {
 		resblks = 0;
 		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
-				XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
+					  XFS_TRANS_PERM_LOG_RES, log_count);
 	}
 	if (error) {
 		ASSERT(error != ENOSPC);
-		REMOVE_DEBUG_TRACE(__LINE__);
-		xfs_trans_cancel(tp, 0);
-		return error;
+		cancel_flags = 0;
+		goto out_trans_cancel;
 	}
 
 	error = xfs_lock_dir_and_entry(dp, ip);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		xfs_trans_cancel(tp, cancel_flags);
-		goto std_return;
-	}
+	if (error)
+		goto out_trans_cancel;
 
 	/*
 	 * At this point, we've gotten both the directory and the entry
@@ -2226,6 +2225,21 @@ xfs_remove(
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 
 	/*
+	 * If we're removing a directory perform some additional validation.
+	 */
+	if (is_dir) {
+		ASSERT(ip->i_d.di_nlink >= 2);
+		if (ip->i_d.di_nlink != 2) {
+			error = XFS_ERROR(ENOTEMPTY);
+			goto out_trans_cancel;
+		}
+		if (!xfs_dir_isempty(ip)) {
+			error = XFS_ERROR(ENOTEMPTY);
+			goto out_trans_cancel;
+		}
+	}
+
+	/*
 	 * Entry must exist since we did a lookup in xfs_lock_dir_and_entry.
 	 */
 	XFS_BMAP_INIT(&free_list, &first_block);
@@ -2233,39 +2247,64 @@ xfs_remove(
 					&first_block, &free_list, resblks);
 	if (error) {
 		ASSERT(error != ENOENT);
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto error1;
+		goto out_bmap_cancel;
 	}
 	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
 
+	/*
+	 * Bump the in memory generation count on the parent
+	 * directory so that other can know that it has changed.
+	 */
 	dp->i_gen++;
 	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
 
-	error = xfs_droplink(tp, ip);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto error1;
+	if (is_dir) {
+		/*
+		 * Drop the link from ip's "..".
+		 */
+		error = xfs_droplink(tp, dp);
+		if (error)
+			goto out_bmap_cancel;
+
+		/*
+		 * Drop the link from dp to ip.
+		 */
+		error = xfs_droplink(tp, ip);
+		if (error)
+			goto out_bmap_cancel;
+	} else {
+		/*
+		 * When removing a non-directory we need to log the parent
+		 * inode here for the i_gen update.  For a directory this is
+		 * done implicitly by the xfs_droplink call for the ".." entry.
+		 */
+		xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
 	}
 
-	/* Determine if this is the last link while
+	/*
+	 * Drop the "." link from ip to self.
+	 */
+	error = xfs_droplink(tp, ip);
+	if (error)
+		goto out_bmap_cancel;
+
+	/*
+	 * Determine if this is the last link while
 	 * we are in the transaction.
 	 */
-	link_zero = (ip)->i_d.di_nlink==0;
+	link_zero = (ip->i_d.di_nlink == 0);
 
 	/*
 	 * If this is a synchronous mount, make sure that the
 	 * remove transaction goes to disk before returning to
 	 * the user.
 	 */
-	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
+	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
 		xfs_trans_set_sync(tp);
-	}
 
 	error = xfs_bmap_finish(&tp, &free_list, &committed);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto error_rele;
-	}
+	if (error)
+		goto out_bmap_cancel;
 
 	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
 	if (error)
@@ -2277,39 +2316,27 @@ xfs_remove(
 	 * will get killed on last close in xfs_close() so we don't
 	 * have to worry about that.
 	 */
-	if (link_zero && xfs_inode_is_filestream(ip))
+	if (!is_dir && link_zero && xfs_inode_is_filestream(ip))
 		xfs_filestream_deassociate(ip);
 
 	xfs_itrace_exit(ip);
+	xfs_itrace_exit(dp);
 
-/*	Fall through to std_return with error = 0 */
  std_return:
 	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
-		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
-				dp, DM_RIGHT_NULL,
-				NULL, DM_RIGHT_NULL,
-				name->name, NULL, ip->i_d.di_mode, error, 0);
+		XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE, dp, DM_RIGHT_NULL,
+				NULL, DM_RIGHT_NULL, name->name, NULL,
+				ip->i_d.di_mode, error, 0);
 	}
+
 	return error;
 
- error1:
+ out_bmap_cancel:
 	xfs_bmap_cancel(&free_list);
 	cancel_flags |= XFS_TRANS_ABORT;
+ out_trans_cancel:
 	xfs_trans_cancel(tp, cancel_flags);
 	goto std_return;
-
- error_rele:
-	/*
-	 * In this case make sure to not release the inode until after
-	 * the current transaction is aborted.  Releasing it beforehand
-	 * can cause us to go to xfs_inactive and start a recursive
-	 * transaction which can easily deadlock with the current one.
-	 */
-	xfs_bmap_cancel(&free_list);
-	cancel_flags |= XFS_TRANS_ABORT;
-	xfs_trans_cancel(tp, cancel_flags);
-
-	goto std_return;
 }
 
 int
@@ -2675,186 +2702,6 @@ std_return:
 }
 
 int
-xfs_rmdir(
-	xfs_inode_t             *dp,
-	struct xfs_name		*name,
-	xfs_inode_t		*cdp)
-{
-	xfs_mount_t		*mp = dp->i_mount;
-	xfs_trans_t             *tp;
-	int                     error;
-	xfs_bmap_free_t         free_list;
-	xfs_fsblock_t           first_block;
-	int			cancel_flags;
-	int			committed;
-	int			last_cdp_link;
-	uint			resblks;
-
-	xfs_itrace_entry(dp);
-
-	if (XFS_FORCED_SHUTDOWN(mp))
-		return XFS_ERROR(EIO);
-
-	if (DM_EVENT_ENABLED(dp, DM_EVENT_REMOVE)) {
-		error = XFS_SEND_NAMESP(mp, DM_EVENT_REMOVE,
-					dp, DM_RIGHT_NULL,
-					NULL, DM_RIGHT_NULL, name->name,
-					NULL, cdp->i_d.di_mode, 0, 0);
-		if (error)
-			return XFS_ERROR(error);
-	}
-
-	/*
-	 * Get the dquots for the inodes.
-	 */
-	error = XFS_QM_DQATTACH(mp, dp, 0);
-	if (!error)
-		error = XFS_QM_DQATTACH(mp, cdp, 0);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto std_return;
-	}
-
-	tp = xfs_trans_alloc(mp, XFS_TRANS_RMDIR);
-	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
-	/*
-	 * We try to get the real space reservation first,
-	 * allowing for directory btree deletion(s) implying
-	 * possible bmap insert(s).  If we can't get the space
-	 * reservation then we use 0 instead, and avoid the bmap
-	 * btree insert(s) in the directory code by, if the bmap
-	 * insert tries to happen, instead trimming the LAST
-	 * block from the directory.
-	 */
-	resblks = XFS_REMOVE_SPACE_RES(mp);
-	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
-			XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
-	if (error == ENOSPC) {
-		resblks = 0;
-		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
-				XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
-	}
-	if (error) {
-		ASSERT(error != ENOSPC);
-		cancel_flags = 0;
-		goto error_return;
-	}
-	XFS_BMAP_INIT(&free_list, &first_block);
-
-	/*
-	 * Now lock the child directory inode and the parent directory
-	 * inode in the proper order.  This will take care of validating
-	 * that the directory entry for the child directory inode has
-	 * not changed while we were obtaining a log reservation.
-	 */
-	error = xfs_lock_dir_and_entry(dp, cdp);
-	if (error) {
-		xfs_trans_cancel(tp, cancel_flags);
-		goto std_return;
-	}
-
-	IHOLD(dp);
-	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
-
-	IHOLD(cdp);
-	xfs_trans_ijoin(tp, cdp, XFS_ILOCK_EXCL);
-
-	ASSERT(cdp->i_d.di_nlink >= 2);
-	if (cdp->i_d.di_nlink != 2) {
-		error = XFS_ERROR(ENOTEMPTY);
-		goto error_return;
-	}
-	if (!xfs_dir_isempty(cdp)) {
-		error = XFS_ERROR(ENOTEMPTY);
-		goto error_return;
-	}
-
-	error = xfs_dir_removename(tp, dp, name, cdp->i_ino,
-					&first_block, &free_list, resblks);
-	if (error)
-		goto error1;
-
-	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
-
-	/*
-	 * Bump the in memory generation count on the parent
-	 * directory so that other can know that it has changed.
-	 */
-	dp->i_gen++;
-
-	/*
-	 * Drop the link from cdp's "..".
-	 */
-	error = xfs_droplink(tp, dp);
-	if (error) {
-		goto error1;
-	}
-
-	/*
-	 * Drop the link from dp to cdp.
-	 */
-	error = xfs_droplink(tp, cdp);
-	if (error) {
-		goto error1;
-	}
-
-	/*
-	 * Drop the "." link from cdp to self.
-	 */
-	error = xfs_droplink(tp, cdp);
-	if (error) {
-		goto error1;
-	}
-
-	/* Determine these before committing transaction */
-	last_cdp_link = (cdp)->i_d.di_nlink==0;
-
-	/*
-	 * If this is a synchronous mount, make sure that the
-	 * rmdir transaction goes to disk before returning to
-	 * the user.
-	 */
-	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
-		xfs_trans_set_sync(tp);
-	}
-
-	error = xfs_bmap_finish (&tp, &free_list, &committed);
-	if (error) {
-		xfs_bmap_cancel(&free_list);
-		xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES |
-				 XFS_TRANS_ABORT));
-		goto std_return;
-	}
-
-	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
-	if (error) {
-		goto std_return;
-	}
-
-
-	/* Fall through to std_return with error = 0 or the errno
-	 * from xfs_trans_commit. */
- std_return:
-	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
-		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
-					dp, DM_RIGHT_NULL,
-					NULL, DM_RIGHT_NULL,
-					name->name, NULL, cdp->i_d.di_mode,
-					error, 0);
-	}
-	return error;
-
- error1:
-	xfs_bmap_cancel(&free_list);
-	cancel_flags |= XFS_TRANS_ABORT;
-	/* FALLTHROUGH */
-
- error_return:
-	xfs_trans_cancel(tp, cancel_flags);
-	goto std_return;
-}
-
-int
 xfs_symlink(
 	xfs_inode_t		*dp,
 	struct xfs_name		*link_name,
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-05-02 00:09:32.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c	2008-05-02 08:30:24.000000000 +0200
@@ -252,8 +252,7 @@ STATIC void
 xfs_cleanup_inode(
 	struct inode	*dir,
 	struct inode	*inode,
-	struct dentry	*dentry,
-	int		mode)
+	struct dentry	*dentry)
 {
 	struct xfs_name	teardown;
 
@@ -264,10 +263,7 @@ xfs_cleanup_inode(
 	 */
 	xfs_dentry_to_name(&teardown, dentry);
 
-	if (S_ISDIR(mode))
-		xfs_rmdir(XFS_I(dir), &teardown, XFS_I(inode));
-	else
-		xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
+	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
 	iput(inode);
 }
 
@@ -349,7 +345,7 @@ xfs_vn_mknod(
 	return -error;
 
  out_cleanup_inode:
-	xfs_cleanup_inode(dir, inode, dentry, mode);
+	xfs_cleanup_inode(dir, inode, dentry);
  out_free_acl:
 	if (default_acl)
 		_ACL_FREE(default_acl);
@@ -478,31 +474,12 @@ xfs_vn_symlink(
 	return 0;
 
  out_cleanup_inode:
-	xfs_cleanup_inode(dir, inode, dentry, 0);
+	xfs_cleanup_inode(dir, inode, dentry);
  out:
 	return -error;
 }
 
 STATIC int
-xfs_vn_rmdir(
-	struct inode	*dir,
-	struct dentry	*dentry)
-{
-	struct inode	*inode = dentry->d_inode;
-	struct xfs_name	name;
-	int		error;
-
-	xfs_dentry_to_name(&name, dentry);
-
-	error = xfs_rmdir(XFS_I(dir), &name, XFS_I(inode));
-	if (likely(!error)) {
-		xfs_validate_fields(inode);
-		xfs_validate_fields(dir);
-	}
-	return -error;
-}
-
-STATIC int
 xfs_vn_rename(
 	struct inode	*odir,
 	struct dentry	*odentry,
@@ -796,7 +773,13 @@ static const struct inode_operations xfs
 	.unlink			= xfs_vn_unlink,
 	.symlink		= xfs_vn_symlink,
 	.mkdir			= xfs_vn_mkdir,
-	.rmdir			= xfs_vn_rmdir,
+	/*
+	 * Yes, XFS uses the same method for rmdir and unlink.
+	 *
+	 * There are some subtile differences deeper in the code,
+	 * but we use S_ISDIR to check for those.
+	 */
+	.rmdir			= xfs_vn_unlink,
 	.mknod			= xfs_vn_mknod,
 	.rename			= xfs_vn_rename,
 	.permission		= xfs_vn_permission,
Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.h
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.h	2008-04-26 20:05:39.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.h	2008-05-02 08:30:24.000000000 +0200
@@ -32,8 +32,6 @@ int xfs_link(struct xfs_inode *tdp, stru
 		struct xfs_name *target_name);
 int xfs_mkdir(struct xfs_inode *dp, struct xfs_name *dir_name,
 		mode_t mode, struct xfs_inode **ipp, struct cred *credp);
-int xfs_rmdir(struct xfs_inode *dp, struct xfs_name *name,
-		struct xfs_inode *cdp);
 int xfs_readdir(struct xfs_inode	*dp, void *dirent, size_t bufsize,
 		       xfs_off_t *offset, filldir_t filldir);
 int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,

Re: [PATCH 1/2] merge xfs_rmdir into xfs_remove

[Christoph Hellwig]

ping?

On Fri, May 02, 2008 at 12:57:57PM +0200, Christoph Hellwig wrote:
> xfs_remove and xfs_rmdir are almost the same with a little more work
> performed in xfs_rmdir due to the . and .. entries.  This patch merges
> xfs_rmdir into xfs_remove and performs these actions conditionally.
> 
> Also clean up the error handling which was a nightmare in both versions
> before.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-05-01 22:56:57.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-05-02 08:30:24.000000000 +0200
> @@ -2135,13 +2135,6 @@ again:
>  #endif
>  }
>  
> -#ifdef	DEBUG
> -#define	REMOVE_DEBUG_TRACE(x)	{remove_which_error_return = (x);}
> -int remove_which_error_return = 0;
> -#else /* ! DEBUG */
> -#define	REMOVE_DEBUG_TRACE(x)
> -#endif	/* ! DEBUG */
> -
>  int
>  xfs_remove(
>  	xfs_inode_t             *dp,
> @@ -2150,6 +2143,7 @@ xfs_remove(
>  {
>  	xfs_mount_t		*mp = dp->i_mount;
>  	xfs_trans_t             *tp = NULL;
> +	int			is_dir = S_ISDIR(ip->i_d.di_mode);
>  	int                     error = 0;
>  	xfs_bmap_free_t         free_list;
>  	xfs_fsblock_t           first_block;
> @@ -2157,8 +2151,11 @@ xfs_remove(
>  	int			committed;
>  	int			link_zero;
>  	uint			resblks;
> +	uint			trans;
> +	uint			log_count;
>  
>  	xfs_itrace_entry(dp);
> +	xfs_itrace_entry(ip);
>  
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return XFS_ERROR(EIO);
> @@ -2171,19 +2168,25 @@ xfs_remove(
>  			return error;
>  	}
>  
> -	xfs_itrace_entry(ip);
> -	xfs_itrace_ref(ip);
> -
>  	error = XFS_QM_DQATTACH(mp, dp, 0);
> -	if (!error)
> -		error = XFS_QM_DQATTACH(mp, ip, 0);
> -	if (error) {
> -		REMOVE_DEBUG_TRACE(__LINE__);
> +	if (error)
> +		goto std_return;
> +
> +	error = XFS_QM_DQATTACH(mp, ip, 0);
> +	if (error)
>  		goto std_return;
> +
> +	if (is_dir) {
> +		trans = XFS_TRANS_RMDIR;
> +		log_count = XFS_DEFAULT_LOG_COUNT;
> +	} else {
> +		trans = XFS_TRANS_REMOVE;
> +		log_count = XFS_REMOVE_LOG_COUNT;
>  	}
>  
> -	tp = xfs_trans_alloc(mp, XFS_TRANS_REMOVE);
> +	tp = xfs_trans_alloc(mp, trans);
>  	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> +
>  	/*
>  	 * We try to get the real space reservation first,
>  	 * allowing for directory btree deletion(s) implying
> @@ -2195,25 +2198,21 @@ xfs_remove(
>  	 */
>  	resblks = XFS_REMOVE_SPACE_RES(mp);
>  	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
> -			XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
> +				  XFS_TRANS_PERM_LOG_RES, log_count);
>  	if (error == ENOSPC) {
>  		resblks = 0;
>  		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
> -				XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
> +					  XFS_TRANS_PERM_LOG_RES, log_count);
>  	}
>  	if (error) {
>  		ASSERT(error != ENOSPC);
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		xfs_trans_cancel(tp, 0);
> -		return error;
> +		cancel_flags = 0;
> +		goto out_trans_cancel;
>  	}
>  
>  	error = xfs_lock_dir_and_entry(dp, ip);
> -	if (error) {
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		xfs_trans_cancel(tp, cancel_flags);
> -		goto std_return;
> -	}
> +	if (error)
> +		goto out_trans_cancel;
>  
>  	/*
>  	 * At this point, we've gotten both the directory and the entry
> @@ -2226,6 +2225,21 @@ xfs_remove(
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  
>  	/*
> +	 * If we're removing a directory perform some additional validation.
> +	 */
> +	if (is_dir) {
> +		ASSERT(ip->i_d.di_nlink >= 2);
> +		if (ip->i_d.di_nlink != 2) {
> +			error = XFS_ERROR(ENOTEMPTY);
> +			goto out_trans_cancel;
> +		}
> +		if (!xfs_dir_isempty(ip)) {
> +			error = XFS_ERROR(ENOTEMPTY);
> +			goto out_trans_cancel;
> +		}
> +	}
> +
> +	/*
>  	 * Entry must exist since we did a lookup in xfs_lock_dir_and_entry.
>  	 */
>  	XFS_BMAP_INIT(&free_list, &first_block);
> @@ -2233,39 +2247,64 @@ xfs_remove(
>  					&first_block, &free_list, resblks);
>  	if (error) {
>  		ASSERT(error != ENOENT);
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		goto error1;
> +		goto out_bmap_cancel;
>  	}
>  	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
>  
> +	/*
> +	 * Bump the in memory generation count on the parent
> +	 * directory so that other can know that it has changed.
> +	 */
>  	dp->i_gen++;
>  	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
>  
> -	error = xfs_droplink(tp, ip);
> -	if (error) {
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		goto error1;
> +	if (is_dir) {
> +		/*
> +		 * Drop the link from ip's "..".
> +		 */
> +		error = xfs_droplink(tp, dp);
> +		if (error)
> +			goto out_bmap_cancel;
> +
> +		/*
> +		 * Drop the link from dp to ip.
> +		 */
> +		error = xfs_droplink(tp, ip);
> +		if (error)
> +			goto out_bmap_cancel;
> +	} else {
> +		/*
> +		 * When removing a non-directory we need to log the parent
> +		 * inode here for the i_gen update.  For a directory this is
> +		 * done implicitly by the xfs_droplink call for the ".." entry.
> +		 */
> +		xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
>  	}
>  
> -	/* Determine if this is the last link while
> +	/*
> +	 * Drop the "." link from ip to self.
> +	 */
> +	error = xfs_droplink(tp, ip);
> +	if (error)
> +		goto out_bmap_cancel;
> +
> +	/*
> +	 * Determine if this is the last link while
>  	 * we are in the transaction.
>  	 */
> -	link_zero = (ip)->i_d.di_nlink==0;
> +	link_zero = (ip->i_d.di_nlink == 0);
>  
>  	/*
>  	 * If this is a synchronous mount, make sure that the
>  	 * remove transaction goes to disk before returning to
>  	 * the user.
>  	 */
> -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
> +	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
>  		xfs_trans_set_sync(tp);
> -	}
>  
>  	error = xfs_bmap_finish(&tp, &free_list, &committed);
> -	if (error) {
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		goto error_rele;
> -	}
> +	if (error)
> +		goto out_bmap_cancel;
>  
>  	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
>  	if (error)
> @@ -2277,39 +2316,27 @@ xfs_remove(
>  	 * will get killed on last close in xfs_close() so we don't
>  	 * have to worry about that.
>  	 */
> -	if (link_zero && xfs_inode_is_filestream(ip))
> +	if (!is_dir && link_zero && xfs_inode_is_filestream(ip))
>  		xfs_filestream_deassociate(ip);
>  
>  	xfs_itrace_exit(ip);
> +	xfs_itrace_exit(dp);
>  
> -/*	Fall through to std_return with error = 0 */
>   std_return:
>  	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
> -		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
> -				dp, DM_RIGHT_NULL,
> -				NULL, DM_RIGHT_NULL,
> -				name->name, NULL, ip->i_d.di_mode, error, 0);
> +		XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE, dp, DM_RIGHT_NULL,
> +				NULL, DM_RIGHT_NULL, name->name, NULL,
> +				ip->i_d.di_mode, error, 0);
>  	}
> +
>  	return error;
>  
> - error1:
> + out_bmap_cancel:
>  	xfs_bmap_cancel(&free_list);
>  	cancel_flags |= XFS_TRANS_ABORT;
> + out_trans_cancel:
>  	xfs_trans_cancel(tp, cancel_flags);
>  	goto std_return;
> -
> - error_rele:
> -	/*
> -	 * In this case make sure to not release the inode until after
> -	 * the current transaction is aborted.  Releasing it beforehand
> -	 * can cause us to go to xfs_inactive and start a recursive
> -	 * transaction which can easily deadlock with the current one.
> -	 */
> -	xfs_bmap_cancel(&free_list);
> -	cancel_flags |= XFS_TRANS_ABORT;
> -	xfs_trans_cancel(tp, cancel_flags);
> -
> -	goto std_return;
>  }
>  
>  int
> @@ -2675,186 +2702,6 @@ std_return:
>  }
>  
>  int
> -xfs_rmdir(
> -	xfs_inode_t             *dp,
> -	struct xfs_name		*name,
> -	xfs_inode_t		*cdp)
> -{
> -	xfs_mount_t		*mp = dp->i_mount;
> -	xfs_trans_t             *tp;
> -	int                     error;
> -	xfs_bmap_free_t         free_list;
> -	xfs_fsblock_t           first_block;
> -	int			cancel_flags;
> -	int			committed;
> -	int			last_cdp_link;
> -	uint			resblks;
> -
> -	xfs_itrace_entry(dp);
> -
> -	if (XFS_FORCED_SHUTDOWN(mp))
> -		return XFS_ERROR(EIO);
> -
> -	if (DM_EVENT_ENABLED(dp, DM_EVENT_REMOVE)) {
> -		error = XFS_SEND_NAMESP(mp, DM_EVENT_REMOVE,
> -					dp, DM_RIGHT_NULL,
> -					NULL, DM_RIGHT_NULL, name->name,
> -					NULL, cdp->i_d.di_mode, 0, 0);
> -		if (error)
> -			return XFS_ERROR(error);
> -	}
> -
> -	/*
> -	 * Get the dquots for the inodes.
> -	 */
> -	error = XFS_QM_DQATTACH(mp, dp, 0);
> -	if (!error)
> -		error = XFS_QM_DQATTACH(mp, cdp, 0);
> -	if (error) {
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		goto std_return;
> -	}
> -
> -	tp = xfs_trans_alloc(mp, XFS_TRANS_RMDIR);
> -	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> -	/*
> -	 * We try to get the real space reservation first,
> -	 * allowing for directory btree deletion(s) implying
> -	 * possible bmap insert(s).  If we can't get the space
> -	 * reservation then we use 0 instead, and avoid the bmap
> -	 * btree insert(s) in the directory code by, if the bmap
> -	 * insert tries to happen, instead trimming the LAST
> -	 * block from the directory.
> -	 */
> -	resblks = XFS_REMOVE_SPACE_RES(mp);
> -	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
> -			XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
> -	if (error == ENOSPC) {
> -		resblks = 0;
> -		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
> -				XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
> -	}
> -	if (error) {
> -		ASSERT(error != ENOSPC);
> -		cancel_flags = 0;
> -		goto error_return;
> -	}
> -	XFS_BMAP_INIT(&free_list, &first_block);
> -
> -	/*
> -	 * Now lock the child directory inode and the parent directory
> -	 * inode in the proper order.  This will take care of validating
> -	 * that the directory entry for the child directory inode has
> -	 * not changed while we were obtaining a log reservation.
> -	 */
> -	error = xfs_lock_dir_and_entry(dp, cdp);
> -	if (error) {
> -		xfs_trans_cancel(tp, cancel_flags);
> -		goto std_return;
> -	}
> -
> -	IHOLD(dp);
> -	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
> -
> -	IHOLD(cdp);
> -	xfs_trans_ijoin(tp, cdp, XFS_ILOCK_EXCL);
> -
> -	ASSERT(cdp->i_d.di_nlink >= 2);
> -	if (cdp->i_d.di_nlink != 2) {
> -		error = XFS_ERROR(ENOTEMPTY);
> -		goto error_return;
> -	}
> -	if (!xfs_dir_isempty(cdp)) {
> -		error = XFS_ERROR(ENOTEMPTY);
> -		goto error_return;
> -	}
> -
> -	error = xfs_dir_removename(tp, dp, name, cdp->i_ino,
> -					&first_block, &free_list, resblks);
> -	if (error)
> -		goto error1;
> -
> -	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> -
> -	/*
> -	 * Bump the in memory generation count on the parent
> -	 * directory so that other can know that it has changed.
> -	 */
> -	dp->i_gen++;
> -
> -	/*
> -	 * Drop the link from cdp's "..".
> -	 */
> -	error = xfs_droplink(tp, dp);
> -	if (error) {
> -		goto error1;
> -	}
> -
> -	/*
> -	 * Drop the link from dp to cdp.
> -	 */
> -	error = xfs_droplink(tp, cdp);
> -	if (error) {
> -		goto error1;
> -	}
> -
> -	/*
> -	 * Drop the "." link from cdp to self.
> -	 */
> -	error = xfs_droplink(tp, cdp);
> -	if (error) {
> -		goto error1;
> -	}
> -
> -	/* Determine these before committing transaction */
> -	last_cdp_link = (cdp)->i_d.di_nlink==0;
> -
> -	/*
> -	 * If this is a synchronous mount, make sure that the
> -	 * rmdir transaction goes to disk before returning to
> -	 * the user.
> -	 */
> -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
> -		xfs_trans_set_sync(tp);
> -	}
> -
> -	error = xfs_bmap_finish (&tp, &free_list, &committed);
> -	if (error) {
> -		xfs_bmap_cancel(&free_list);
> -		xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES |
> -				 XFS_TRANS_ABORT));
> -		goto std_return;
> -	}
> -
> -	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> -	if (error) {
> -		goto std_return;
> -	}
> -
> -
> -	/* Fall through to std_return with error = 0 or the errno
> -	 * from xfs_trans_commit. */
> - std_return:
> -	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
> -		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
> -					dp, DM_RIGHT_NULL,
> -					NULL, DM_RIGHT_NULL,
> -					name->name, NULL, cdp->i_d.di_mode,
> -					error, 0);
> -	}
> -	return error;
> -
> - error1:
> -	xfs_bmap_cancel(&free_list);
> -	cancel_flags |= XFS_TRANS_ABORT;
> -	/* FALLTHROUGH */
> -
> - error_return:
> -	xfs_trans_cancel(tp, cancel_flags);
> -	goto std_return;
> -}
> -
> -int
>  xfs_symlink(
>  	xfs_inode_t		*dp,
>  	struct xfs_name		*link_name,
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-05-02 00:09:32.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c	2008-05-02 08:30:24.000000000 +0200
> @@ -252,8 +252,7 @@ STATIC void
>  xfs_cleanup_inode(
>  	struct inode	*dir,
>  	struct inode	*inode,
> -	struct dentry	*dentry,
> -	int		mode)
> +	struct dentry	*dentry)
>  {
>  	struct xfs_name	teardown;
>  
> @@ -264,10 +263,7 @@ xfs_cleanup_inode(
>  	 */
>  	xfs_dentry_to_name(&teardown, dentry);
>  
> -	if (S_ISDIR(mode))
> -		xfs_rmdir(XFS_I(dir), &teardown, XFS_I(inode));
> -	else
> -		xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
> +	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
>  	iput(inode);
>  }
>  
> @@ -349,7 +345,7 @@ xfs_vn_mknod(
>  	return -error;
>  
>   out_cleanup_inode:
> -	xfs_cleanup_inode(dir, inode, dentry, mode);
> +	xfs_cleanup_inode(dir, inode, dentry);
>   out_free_acl:
>  	if (default_acl)
>  		_ACL_FREE(default_acl);
> @@ -478,31 +474,12 @@ xfs_vn_symlink(
>  	return 0;
>  
>   out_cleanup_inode:
> -	xfs_cleanup_inode(dir, inode, dentry, 0);
> +	xfs_cleanup_inode(dir, inode, dentry);
>   out:
>  	return -error;
>  }
>  
>  STATIC int
> -xfs_vn_rmdir(
> -	struct inode	*dir,
> -	struct dentry	*dentry)
> -{
> -	struct inode	*inode = dentry->d_inode;
> -	struct xfs_name	name;
> -	int		error;
> -
> -	xfs_dentry_to_name(&name, dentry);
> -
> -	error = xfs_rmdir(XFS_I(dir), &name, XFS_I(inode));
> -	if (likely(!error)) {
> -		xfs_validate_fields(inode);
> -		xfs_validate_fields(dir);
> -	}
> -	return -error;
> -}
> -
> -STATIC int
>  xfs_vn_rename(
>  	struct inode	*odir,
>  	struct dentry	*odentry,
> @@ -796,7 +773,13 @@ static const struct inode_operations xfs
>  	.unlink			= xfs_vn_unlink,
>  	.symlink		= xfs_vn_symlink,
>  	.mkdir			= xfs_vn_mkdir,
> -	.rmdir			= xfs_vn_rmdir,
> +	/*
> +	 * Yes, XFS uses the same method for rmdir and unlink.
> +	 *
> +	 * There are some subtile differences deeper in the code,
> +	 * but we use S_ISDIR to check for those.
> +	 */
> +	.rmdir			= xfs_vn_unlink,
>  	.mknod			= xfs_vn_mknod,
>  	.rename			= xfs_vn_rename,
>  	.permission		= xfs_vn_permission,
> Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.h	2008-04-26 20:05:39.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.h	2008-05-02 08:30:24.000000000 +0200
> @@ -32,8 +32,6 @@ int xfs_link(struct xfs_inode *tdp, stru
>  		struct xfs_name *target_name);
>  int xfs_mkdir(struct xfs_inode *dp, struct xfs_name *dir_name,
>  		mode_t mode, struct xfs_inode **ipp, struct cred *credp);
> -int xfs_rmdir(struct xfs_inode *dp, struct xfs_name *name,
> -		struct xfs_inode *cdp);
>  int xfs_readdir(struct xfs_inode	*dp, void *dirent, size_t bufsize,
>  		       xfs_off_t *offset, filldir_t filldir);
>  int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,
---end quoted text---

Re: [PATCH 1/2] merge xfs_rmdir into xfs_remove

[Christoph Hellwig]

On Fri, May 02, 2008 at 12:57:57PM +0200, Christoph Hellwig wrote:
> xfs_remove and xfs_rmdir are almost the same with a little more work
> performed in xfs_rmdir due to the . and .. entries.  This patch merges
> xfs_rmdir into xfs_remove and performs these actions conditionally.
> 
> Also clean up the error handling which was a nightmare in both versions
> before.

Updated for the case-insensitive filename changes.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-05-21 10:10:30.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-05-21 10:21:31.000000000 +0200
@@ -2116,13 +2116,6 @@ again:
 #endif
 }
 
-#ifdef	DEBUG
-#define	REMOVE_DEBUG_TRACE(x)	{remove_which_error_return = (x);}
-int remove_which_error_return = 0;
-#else /* ! DEBUG */
-#define	REMOVE_DEBUG_TRACE(x)
-#endif	/* ! DEBUG */
-
 int
 xfs_remove(
 	xfs_inode_t             *dp,
@@ -2131,6 +2124,7 @@ xfs_remove(
 {
 	xfs_mount_t		*mp = dp->i_mount;
 	xfs_trans_t             *tp = NULL;
+	int			is_dir = S_ISDIR(ip->i_d.di_mode);
 	int                     error = 0;
 	xfs_bmap_free_t         free_list;
 	xfs_fsblock_t           first_block;
@@ -2138,8 +2132,11 @@ xfs_remove(
 	int			committed;
 	int			link_zero;
 	uint			resblks;
+	uint			trans;
+	uint			log_count;
 
 	xfs_itrace_entry(dp);
+	xfs_itrace_entry(ip);
 
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
@@ -2152,19 +2149,25 @@ xfs_remove(
 			return error;
 	}
 
-	xfs_itrace_entry(ip);
-	xfs_itrace_ref(ip);
-
 	error = XFS_QM_DQATTACH(mp, dp, 0);
-	if (!error)
-		error = XFS_QM_DQATTACH(mp, ip, 0);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
+	if (error)
+		goto std_return;
+
+	error = XFS_QM_DQATTACH(mp, ip, 0);
+	if (error)
 		goto std_return;
+
+	if (is_dir) {
+		trans = XFS_TRANS_RMDIR;
+		log_count = XFS_DEFAULT_LOG_COUNT;
+	} else {
+		trans = XFS_TRANS_REMOVE;
+		log_count = XFS_REMOVE_LOG_COUNT;
 	}
 
-	tp = xfs_trans_alloc(mp, XFS_TRANS_REMOVE);
+	tp = xfs_trans_alloc(mp, trans);
 	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
+
 	/*
 	 * We try to get the real space reservation first,
 	 * allowing for directory btree deletion(s) implying
@@ -2176,25 +2179,21 @@ xfs_remove(
 	 */
 	resblks = XFS_REMOVE_SPACE_RES(mp);
 	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
-			XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
+				  XFS_TRANS_PERM_LOG_RES, log_count);
 	if (error == ENOSPC) {
 		resblks = 0;
 		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
-				XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
+					  XFS_TRANS_PERM_LOG_RES, log_count);
 	}
 	if (error) {
 		ASSERT(error != ENOSPC);
-		REMOVE_DEBUG_TRACE(__LINE__);
-		xfs_trans_cancel(tp, 0);
-		return error;
+		cancel_flags = 0;
+		goto out_trans_cancel;
 	}
 
 	error = xfs_lock_dir_and_entry(dp, ip);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		xfs_trans_cancel(tp, cancel_flags);
-		goto std_return;
-	}
+	if (error)
+		goto out_trans_cancel;
 
 	/*
 	 * At this point, we've gotten both the directory and the entry
@@ -2207,6 +2206,21 @@ xfs_remove(
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 
 	/*
+	 * If we're removing a directory perform some additional validation.
+	 */
+	if (is_dir) {
+		ASSERT(ip->i_d.di_nlink >= 2);
+		if (ip->i_d.di_nlink != 2) {
+			error = XFS_ERROR(ENOTEMPTY);
+			goto out_trans_cancel;
+		}
+		if (!xfs_dir_isempty(ip)) {
+			error = XFS_ERROR(ENOTEMPTY);
+			goto out_trans_cancel;
+		}
+	}
+
+	/*
 	 * Entry must exist since we did a lookup in xfs_lock_dir_and_entry.
 	 */
 	XFS_BMAP_INIT(&free_list, &first_block);
@@ -2214,39 +2228,64 @@ xfs_remove(
 					&first_block, &free_list, resblks);
 	if (error) {
 		ASSERT(error != ENOENT);
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto error1;
+		goto out_bmap_cancel;
 	}
 	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
 
+	/*
+	 * Bump the in memory generation count on the parent
+	 * directory so that other can know that it has changed.
+	 */
 	dp->i_gen++;
 	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
 
-	error = xfs_droplink(tp, ip);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto error1;
+	if (is_dir) {
+		/*
+		 * Drop the link from ip's "..".
+		 */
+		error = xfs_droplink(tp, dp);
+		if (error)
+			goto out_bmap_cancel;
+
+		/*
+		 * Drop the link from dp to ip.
+		 */
+		error = xfs_droplink(tp, ip);
+		if (error)
+			goto out_bmap_cancel;
+	} else {
+		/*
+		 * When removing a non-directory we need to log the parent
+		 * inode here for the i_gen update.  For a directory this is
+		 * done implicitly by the xfs_droplink call for the ".." entry.
+		 */
+		xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
 	}
 
-	/* Determine if this is the last link while
+	/*
+	 * Drop the "." link from ip to self.
+	 */
+	error = xfs_droplink(tp, ip);
+	if (error)
+		goto out_bmap_cancel;
+
+	/*
+	 * Determine if this is the last link while
 	 * we are in the transaction.
 	 */
-	link_zero = (ip)->i_d.di_nlink==0;
+	link_zero = (ip->i_d.di_nlink == 0);
 
 	/*
 	 * If this is a synchronous mount, make sure that the
 	 * remove transaction goes to disk before returning to
 	 * the user.
 	 */
-	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
+	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
 		xfs_trans_set_sync(tp);
-	}
 
 	error = xfs_bmap_finish(&tp, &free_list, &committed);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto error_rele;
-	}
+	if (error)
+		goto out_bmap_cancel;
 
 	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
 	if (error)
@@ -2258,39 +2297,27 @@ xfs_remove(
 	 * will get killed on last close in xfs_close() so we don't
 	 * have to worry about that.
 	 */
-	if (link_zero && xfs_inode_is_filestream(ip))
+	if (!is_dir && link_zero && xfs_inode_is_filestream(ip))
 		xfs_filestream_deassociate(ip);
 
 	xfs_itrace_exit(ip);
+	xfs_itrace_exit(dp);
 
-/*	Fall through to std_return with error = 0 */
  std_return:
 	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
-		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
-				dp, DM_RIGHT_NULL,
-				NULL, DM_RIGHT_NULL,
-				name->name, NULL, ip->i_d.di_mode, error, 0);
+		XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE, dp, DM_RIGHT_NULL,
+				NULL, DM_RIGHT_NULL, name->name, NULL,
+				ip->i_d.di_mode, error, 0);
 	}
+
 	return error;
 
- error1:
+ out_bmap_cancel:
 	xfs_bmap_cancel(&free_list);
 	cancel_flags |= XFS_TRANS_ABORT;
+ out_trans_cancel:
 	xfs_trans_cancel(tp, cancel_flags);
 	goto std_return;
-
- error_rele:
-	/*
-	 * In this case make sure to not release the inode until after
-	 * the current transaction is aborted.  Releasing it beforehand
-	 * can cause us to go to xfs_inactive and start a recursive
-	 * transaction which can easily deadlock with the current one.
-	 */
-	xfs_bmap_cancel(&free_list);
-	cancel_flags |= XFS_TRANS_ABORT;
-	xfs_trans_cancel(tp, cancel_flags);
-
-	goto std_return;
 }
 
 int
@@ -2656,186 +2683,6 @@ std_return:
 }
 
 int
-xfs_rmdir(
-	xfs_inode_t             *dp,
-	struct xfs_name		*name,
-	xfs_inode_t		*cdp)
-{
-	xfs_mount_t		*mp = dp->i_mount;
-	xfs_trans_t             *tp;
-	int                     error;
-	xfs_bmap_free_t         free_list;
-	xfs_fsblock_t           first_block;
-	int			cancel_flags;
-	int			committed;
-	int			last_cdp_link;
-	uint			resblks;
-
-	xfs_itrace_entry(dp);
-
-	if (XFS_FORCED_SHUTDOWN(mp))
-		return XFS_ERROR(EIO);
-
-	if (DM_EVENT_ENABLED(dp, DM_EVENT_REMOVE)) {
-		error = XFS_SEND_NAMESP(mp, DM_EVENT_REMOVE,
-					dp, DM_RIGHT_NULL,
-					NULL, DM_RIGHT_NULL, name->name,
-					NULL, cdp->i_d.di_mode, 0, 0);
-		if (error)
-			return XFS_ERROR(error);
-	}
-
-	/*
-	 * Get the dquots for the inodes.
-	 */
-	error = XFS_QM_DQATTACH(mp, dp, 0);
-	if (!error)
-		error = XFS_QM_DQATTACH(mp, cdp, 0);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto std_return;
-	}
-
-	tp = xfs_trans_alloc(mp, XFS_TRANS_RMDIR);
-	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
-	/*
-	 * We try to get the real space reservation first,
-	 * allowing for directory btree deletion(s) implying
-	 * possible bmap insert(s).  If we can't get the space
-	 * reservation then we use 0 instead, and avoid the bmap
-	 * btree insert(s) in the directory code by, if the bmap
-	 * insert tries to happen, instead trimming the LAST
-	 * block from the directory.
-	 */
-	resblks = XFS_REMOVE_SPACE_RES(mp);
-	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
-			XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
-	if (error == ENOSPC) {
-		resblks = 0;
-		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
-				XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
-	}
-	if (error) {
-		ASSERT(error != ENOSPC);
-		cancel_flags = 0;
-		goto error_return;
-	}
-	XFS_BMAP_INIT(&free_list, &first_block);
-
-	/*
-	 * Now lock the child directory inode and the parent directory
-	 * inode in the proper order.  This will take care of validating
-	 * that the directory entry for the child directory inode has
-	 * not changed while we were obtaining a log reservation.
-	 */
-	error = xfs_lock_dir_and_entry(dp, cdp);
-	if (error) {
-		xfs_trans_cancel(tp, cancel_flags);
-		goto std_return;
-	}
-
-	IHOLD(dp);
-	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
-
-	IHOLD(cdp);
-	xfs_trans_ijoin(tp, cdp, XFS_ILOCK_EXCL);
-
-	ASSERT(cdp->i_d.di_nlink >= 2);
-	if (cdp->i_d.di_nlink != 2) {
-		error = XFS_ERROR(ENOTEMPTY);
-		goto error_return;
-	}
-	if (!xfs_dir_isempty(cdp)) {
-		error = XFS_ERROR(ENOTEMPTY);
-		goto error_return;
-	}
-
-	error = xfs_dir_removename(tp, dp, name, cdp->i_ino,
-					&first_block, &free_list, resblks);
-	if (error)
-		goto error1;
-
-	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
-
-	/*
-	 * Bump the in memory generation count on the parent
-	 * directory so that other can know that it has changed.
-	 */
-	dp->i_gen++;
-
-	/*
-	 * Drop the link from cdp's "..".
-	 */
-	error = xfs_droplink(tp, dp);
-	if (error) {
-		goto error1;
-	}
-
-	/*
-	 * Drop the link from dp to cdp.
-	 */
-	error = xfs_droplink(tp, cdp);
-	if (error) {
-		goto error1;
-	}
-
-	/*
-	 * Drop the "." link from cdp to self.
-	 */
-	error = xfs_droplink(tp, cdp);
-	if (error) {
-		goto error1;
-	}
-
-	/* Determine these before committing transaction */
-	last_cdp_link = (cdp)->i_d.di_nlink==0;
-
-	/*
-	 * If this is a synchronous mount, make sure that the
-	 * rmdir transaction goes to disk before returning to
-	 * the user.
-	 */
-	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
-		xfs_trans_set_sync(tp);
-	}
-
-	error = xfs_bmap_finish (&tp, &free_list, &committed);
-	if (error) {
-		xfs_bmap_cancel(&free_list);
-		xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES |
-				 XFS_TRANS_ABORT));
-		goto std_return;
-	}
-
-	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
-	if (error) {
-		goto std_return;
-	}
-
-
-	/* Fall through to std_return with error = 0 or the errno
-	 * from xfs_trans_commit. */
- std_return:
-	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
-		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
-					dp, DM_RIGHT_NULL,
-					NULL, DM_RIGHT_NULL,
-					name->name, NULL, cdp->i_d.di_mode,
-					error, 0);
-	}
-	return error;
-
- error1:
-	xfs_bmap_cancel(&free_list);
-	cancel_flags |= XFS_TRANS_ABORT;
-	/* FALLTHROUGH */
-
- error_return:
-	xfs_trans_cancel(tp, cancel_flags);
-	goto std_return;
-}
-
-int
 xfs_symlink(
 	xfs_inode_t		*dp,
 	struct xfs_name		*link_name,
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-05-21 10:17:36.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c	2008-05-21 10:21:53.000000000 +0200
@@ -252,8 +252,7 @@ STATIC void
 xfs_cleanup_inode(
 	struct inode	*dir,
 	struct inode	*inode,
-	struct dentry	*dentry,
-	int		mode)
+	struct dentry	*dentry)
 {
 	struct xfs_name	teardown;
 
@@ -264,10 +263,7 @@ xfs_cleanup_inode(
 	 */
 	xfs_dentry_to_name(&teardown, dentry);
 
-	if (S_ISDIR(mode))
-		xfs_rmdir(XFS_I(dir), &teardown, XFS_I(inode));
-	else
-		xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
+	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
 	iput(inode);
 }
 
@@ -349,7 +345,7 @@ xfs_vn_mknod(
 	return -error;
 
  out_cleanup_inode:
-	xfs_cleanup_inode(dir, inode, dentry, mode);
+	xfs_cleanup_inode(dir, inode, dentry);
  out_free_acl:
 	if (default_acl)
 		_ACL_FREE(default_acl);
@@ -514,31 +510,12 @@ xfs_vn_symlink(
 	return 0;
 
  out_cleanup_inode:
-	xfs_cleanup_inode(dir, inode, dentry, 0);
+	xfs_cleanup_inode(dir, inode, dentry);
  out:
 	return -error;
 }
 
 STATIC int
-xfs_vn_rmdir(
-	struct inode	*dir,
-	struct dentry	*dentry)
-{
-	struct inode	*inode = dentry->d_inode;
-	struct xfs_name	name;
-	int		error;
-
-	xfs_dentry_to_name(&name, dentry);
-
-	error = xfs_rmdir(XFS_I(dir), &name, XFS_I(inode));
-	if (likely(!error)) {
-		xfs_validate_fields(inode);
-		xfs_validate_fields(dir);
-	}
-	return -error;
-}
-
-STATIC int
 xfs_vn_rename(
 	struct inode	*odir,
 	struct dentry	*odentry,
@@ -832,7 +809,13 @@ static const struct inode_operations xfs
 	.unlink			= xfs_vn_unlink,
 	.symlink		= xfs_vn_symlink,
 	.mkdir			= xfs_vn_mkdir,
-	.rmdir			= xfs_vn_rmdir,
+	/*
+	 * Yes, XFS uses the same method for rmdir and unlink.
+	 *
+	 * There are some subtile differences deeper in the code,
+	 * but we use S_ISDIR to check for those.
+	 */
+	.rmdir			= xfs_vn_unlink,
 	.mknod			= xfs_vn_mknod,
 	.rename			= xfs_vn_rename,
 	.permission		= xfs_vn_permission,
@@ -851,7 +834,13 @@ static const struct inode_operations xfs
 	.unlink			= xfs_vn_unlink,
 	.symlink		= xfs_vn_symlink,
 	.mkdir			= xfs_vn_mkdir,
-	.rmdir			= xfs_vn_rmdir,
+	/*
+	 * Yes, XFS uses the same method for rmdir and unlink.
+	 *
+	 * There are some subtile differences deeper in the code,
+	 * but we use S_ISDIR to check for those.
+	 */
+	.rmdir			= xfs_vn_unlink,
 	.mknod			= xfs_vn_mknod,
 	.rename			= xfs_vn_rename,
 	.permission		= xfs_vn_permission,
Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.h
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.h	2008-05-21 10:10:30.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.h	2008-05-21 10:21:31.000000000 +0200
@@ -31,8 +31,6 @@ int xfs_link(struct xfs_inode *tdp, stru
 		struct xfs_name *target_name);
 int xfs_mkdir(struct xfs_inode *dp, struct xfs_name *dir_name,
 		mode_t mode, struct xfs_inode **ipp, struct cred *credp);
-int xfs_rmdir(struct xfs_inode *dp, struct xfs_name *name,
-		struct xfs_inode *cdp);
 int xfs_readdir(struct xfs_inode	*dp, void *dirent, size_t bufsize,
 		       xfs_off_t *offset, filldir_t filldir);
 int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,

Re: [PATCH 1/2] merge xfs_rmdir into xfs_remove

[Christoph Hellwig]

On Wed, May 21, 2008 at 10:25:48AM +0200, Christoph Hellwig wrote:
> On Fri, May 02, 2008 at 12:57:57PM +0200, Christoph Hellwig wrote:
> > xfs_remove and xfs_rmdir are almost the same with a little more work
> > performed in xfs_rmdir due to the . and .. entries.  This patch merges
> > xfs_rmdir into xfs_remove and performs these actions conditionally.
> > 
> > Also clean up the error handling which was a nightmare in both versions
> > before.
> 
> Updated for the case-insensitive filename changes.

And another update for the d_invalidate added on unlink/rmdir.

Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-06-16 08:11:12.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-06-16 08:14:16.000000000 +0200
@@ -2116,13 +2116,6 @@ again:
 #endif
 }
 
-#ifdef	DEBUG
-#define	REMOVE_DEBUG_TRACE(x)	{remove_which_error_return = (x);}
-int remove_which_error_return = 0;
-#else /* ! DEBUG */
-#define	REMOVE_DEBUG_TRACE(x)
-#endif	/* ! DEBUG */
-
 int
 xfs_remove(
 	xfs_inode_t             *dp,
@@ -2131,6 +2124,7 @@ xfs_remove(
 {
 	xfs_mount_t		*mp = dp->i_mount;
 	xfs_trans_t             *tp = NULL;
+	int			is_dir = S_ISDIR(ip->i_d.di_mode);
 	int                     error = 0;
 	xfs_bmap_free_t         free_list;
 	xfs_fsblock_t           first_block;
@@ -2138,8 +2132,11 @@ xfs_remove(
 	int			committed;
 	int			link_zero;
 	uint			resblks;
+	uint			trans;
+	uint			log_count;
 
 	xfs_itrace_entry(dp);
+	xfs_itrace_entry(ip);
 
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
@@ -2152,19 +2149,25 @@ xfs_remove(
 			return error;
 	}
 
-	xfs_itrace_entry(ip);
-	xfs_itrace_ref(ip);
-
 	error = XFS_QM_DQATTACH(mp, dp, 0);
-	if (!error)
-		error = XFS_QM_DQATTACH(mp, ip, 0);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
+	if (error)
+		goto std_return;
+
+	error = XFS_QM_DQATTACH(mp, ip, 0);
+	if (error)
 		goto std_return;
+
+	if (is_dir) {
+		trans = XFS_TRANS_RMDIR;
+		log_count = XFS_DEFAULT_LOG_COUNT;
+	} else {
+		trans = XFS_TRANS_REMOVE;
+		log_count = XFS_REMOVE_LOG_COUNT;
 	}
 
-	tp = xfs_trans_alloc(mp, XFS_TRANS_REMOVE);
+	tp = xfs_trans_alloc(mp, trans);
 	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
+
 	/*
 	 * We try to get the real space reservation first,
 	 * allowing for directory btree deletion(s) implying
@@ -2176,25 +2179,21 @@ xfs_remove(
 	 */
 	resblks = XFS_REMOVE_SPACE_RES(mp);
 	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
-			XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
+				  XFS_TRANS_PERM_LOG_RES, log_count);
 	if (error == ENOSPC) {
 		resblks = 0;
 		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
-				XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
+					  XFS_TRANS_PERM_LOG_RES, log_count);
 	}
 	if (error) {
 		ASSERT(error != ENOSPC);
-		REMOVE_DEBUG_TRACE(__LINE__);
-		xfs_trans_cancel(tp, 0);
-		return error;
+		cancel_flags = 0;
+		goto out_trans_cancel;
 	}
 
 	error = xfs_lock_dir_and_entry(dp, ip);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		xfs_trans_cancel(tp, cancel_flags);
-		goto std_return;
-	}
+	if (error)
+		goto out_trans_cancel;
 
 	/*
 	 * At this point, we've gotten both the directory and the entry
@@ -2207,6 +2206,21 @@ xfs_remove(
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 
 	/*
+	 * If we're removing a directory perform some additional validation.
+	 */
+	if (is_dir) {
+		ASSERT(ip->i_d.di_nlink >= 2);
+		if (ip->i_d.di_nlink != 2) {
+			error = XFS_ERROR(ENOTEMPTY);
+			goto out_trans_cancel;
+		}
+		if (!xfs_dir_isempty(ip)) {
+			error = XFS_ERROR(ENOTEMPTY);
+			goto out_trans_cancel;
+		}
+	}
+
+	/*
 	 * Entry must exist since we did a lookup in xfs_lock_dir_and_entry.
 	 */
 	XFS_BMAP_INIT(&free_list, &first_block);
@@ -2214,39 +2228,64 @@ xfs_remove(
 					&first_block, &free_list, resblks);
 	if (error) {
 		ASSERT(error != ENOENT);
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto error1;
+		goto out_bmap_cancel;
 	}
 	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
 
+	/*
+	 * Bump the in memory generation count on the parent
+	 * directory so that other can know that it has changed.
+	 */
 	dp->i_gen++;
 	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
 
-	error = xfs_droplink(tp, ip);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto error1;
+	if (is_dir) {
+		/*
+		 * Drop the link from ip's "..".
+		 */
+		error = xfs_droplink(tp, dp);
+		if (error)
+			goto out_bmap_cancel;
+
+		/*
+		 * Drop the link from dp to ip.
+		 */
+		error = xfs_droplink(tp, ip);
+		if (error)
+			goto out_bmap_cancel;
+	} else {
+		/*
+		 * When removing a non-directory we need to log the parent
+		 * inode here for the i_gen update.  For a directory this is
+		 * done implicitly by the xfs_droplink call for the ".." entry.
+		 */
+		xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
 	}
 
-	/* Determine if this is the last link while
+	/*
+	 * Drop the "." link from ip to self.
+	 */
+	error = xfs_droplink(tp, ip);
+	if (error)
+		goto out_bmap_cancel;
+
+	/*
+	 * Determine if this is the last link while
 	 * we are in the transaction.
 	 */
-	link_zero = (ip)->i_d.di_nlink==0;
+	link_zero = (ip->i_d.di_nlink == 0);
 
 	/*
 	 * If this is a synchronous mount, make sure that the
 	 * remove transaction goes to disk before returning to
 	 * the user.
 	 */
-	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
+	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
 		xfs_trans_set_sync(tp);
-	}
 
 	error = xfs_bmap_finish(&tp, &free_list, &committed);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto error_rele;
-	}
+	if (error)
+		goto out_bmap_cancel;
 
 	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
 	if (error)
@@ -2258,39 +2297,27 @@ xfs_remove(
 	 * will get killed on last close in xfs_close() so we don't
 	 * have to worry about that.
 	 */
-	if (link_zero && xfs_inode_is_filestream(ip))
+	if (!is_dir && link_zero && xfs_inode_is_filestream(ip))
 		xfs_filestream_deassociate(ip);
 
 	xfs_itrace_exit(ip);
+	xfs_itrace_exit(dp);
 
-/*	Fall through to std_return with error = 0 */
  std_return:
 	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
-		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
-				dp, DM_RIGHT_NULL,
-				NULL, DM_RIGHT_NULL,
-				name->name, NULL, ip->i_d.di_mode, error, 0);
+		XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE, dp, DM_RIGHT_NULL,
+				NULL, DM_RIGHT_NULL, name->name, NULL,
+				ip->i_d.di_mode, error, 0);
 	}
+
 	return error;
 
- error1:
+ out_bmap_cancel:
 	xfs_bmap_cancel(&free_list);
 	cancel_flags |= XFS_TRANS_ABORT;
+ out_trans_cancel:
 	xfs_trans_cancel(tp, cancel_flags);
 	goto std_return;
-
- error_rele:
-	/*
-	 * In this case make sure to not release the inode until after
-	 * the current transaction is aborted.  Releasing it beforehand
-	 * can cause us to go to xfs_inactive and start a recursive
-	 * transaction which can easily deadlock with the current one.
-	 */
-	xfs_bmap_cancel(&free_list);
-	cancel_flags |= XFS_TRANS_ABORT;
-	xfs_trans_cancel(tp, cancel_flags);
-
-	goto std_return;
 }
 
 int
@@ -2656,186 +2683,6 @@ std_return:
 }
 
 int
-xfs_rmdir(
-	xfs_inode_t             *dp,
-	struct xfs_name		*name,
-	xfs_inode_t		*cdp)
-{
-	xfs_mount_t		*mp = dp->i_mount;
-	xfs_trans_t             *tp;
-	int                     error;
-	xfs_bmap_free_t         free_list;
-	xfs_fsblock_t           first_block;
-	int			cancel_flags;
-	int			committed;
-	int			last_cdp_link;
-	uint			resblks;
-
-	xfs_itrace_entry(dp);
-
-	if (XFS_FORCED_SHUTDOWN(mp))
-		return XFS_ERROR(EIO);
-
-	if (DM_EVENT_ENABLED(dp, DM_EVENT_REMOVE)) {
-		error = XFS_SEND_NAMESP(mp, DM_EVENT_REMOVE,
-					dp, DM_RIGHT_NULL,
-					NULL, DM_RIGHT_NULL, name->name,
-					NULL, cdp->i_d.di_mode, 0, 0);
-		if (error)
-			return XFS_ERROR(error);
-	}
-
-	/*
-	 * Get the dquots for the inodes.
-	 */
-	error = XFS_QM_DQATTACH(mp, dp, 0);
-	if (!error)
-		error = XFS_QM_DQATTACH(mp, cdp, 0);
-	if (error) {
-		REMOVE_DEBUG_TRACE(__LINE__);
-		goto std_return;
-	}
-
-	tp = xfs_trans_alloc(mp, XFS_TRANS_RMDIR);
-	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
-	/*
-	 * We try to get the real space reservation first,
-	 * allowing for directory btree deletion(s) implying
-	 * possible bmap insert(s).  If we can't get the space
-	 * reservation then we use 0 instead, and avoid the bmap
-	 * btree insert(s) in the directory code by, if the bmap
-	 * insert tries to happen, instead trimming the LAST
-	 * block from the directory.
-	 */
-	resblks = XFS_REMOVE_SPACE_RES(mp);
-	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
-			XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
-	if (error == ENOSPC) {
-		resblks = 0;
-		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
-				XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
-	}
-	if (error) {
-		ASSERT(error != ENOSPC);
-		cancel_flags = 0;
-		goto error_return;
-	}
-	XFS_BMAP_INIT(&free_list, &first_block);
-
-	/*
-	 * Now lock the child directory inode and the parent directory
-	 * inode in the proper order.  This will take care of validating
-	 * that the directory entry for the child directory inode has
-	 * not changed while we were obtaining a log reservation.
-	 */
-	error = xfs_lock_dir_and_entry(dp, cdp);
-	if (error) {
-		xfs_trans_cancel(tp, cancel_flags);
-		goto std_return;
-	}
-
-	IHOLD(dp);
-	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
-
-	IHOLD(cdp);
-	xfs_trans_ijoin(tp, cdp, XFS_ILOCK_EXCL);
-
-	ASSERT(cdp->i_d.di_nlink >= 2);
-	if (cdp->i_d.di_nlink != 2) {
-		error = XFS_ERROR(ENOTEMPTY);
-		goto error_return;
-	}
-	if (!xfs_dir_isempty(cdp)) {
-		error = XFS_ERROR(ENOTEMPTY);
-		goto error_return;
-	}
-
-	error = xfs_dir_removename(tp, dp, name, cdp->i_ino,
-					&first_block, &free_list, resblks);
-	if (error)
-		goto error1;
-
-	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
-
-	/*
-	 * Bump the in memory generation count on the parent
-	 * directory so that other can know that it has changed.
-	 */
-	dp->i_gen++;
-
-	/*
-	 * Drop the link from cdp's "..".
-	 */
-	error = xfs_droplink(tp, dp);
-	if (error) {
-		goto error1;
-	}
-
-	/*
-	 * Drop the link from dp to cdp.
-	 */
-	error = xfs_droplink(tp, cdp);
-	if (error) {
-		goto error1;
-	}
-
-	/*
-	 * Drop the "." link from cdp to self.
-	 */
-	error = xfs_droplink(tp, cdp);
-	if (error) {
-		goto error1;
-	}
-
-	/* Determine these before committing transaction */
-	last_cdp_link = (cdp)->i_d.di_nlink==0;
-
-	/*
-	 * If this is a synchronous mount, make sure that the
-	 * rmdir transaction goes to disk before returning to
-	 * the user.
-	 */
-	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
-		xfs_trans_set_sync(tp);
-	}
-
-	error = xfs_bmap_finish (&tp, &free_list, &committed);
-	if (error) {
-		xfs_bmap_cancel(&free_list);
-		xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES |
-				 XFS_TRANS_ABORT));
-		goto std_return;
-	}
-
-	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
-	if (error) {
-		goto std_return;
-	}
-
-
-	/* Fall through to std_return with error = 0 or the errno
-	 * from xfs_trans_commit. */
- std_return:
-	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
-		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
-					dp, DM_RIGHT_NULL,
-					NULL, DM_RIGHT_NULL,
-					name->name, NULL, cdp->i_d.di_mode,
-					error, 0);
-	}
-	return error;
-
- error1:
-	xfs_bmap_cancel(&free_list);
-	cancel_flags |= XFS_TRANS_ABORT;
-	/* FALLTHROUGH */
-
- error_return:
-	xfs_trans_cancel(tp, cancel_flags);
-	goto std_return;
-}
-
-int
 xfs_symlink(
 	xfs_inode_t		*dp,
 	struct xfs_name		*link_name,
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-06-16 08:14:12.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c	2008-06-16 08:15:00.000000000 +0200
@@ -252,8 +252,7 @@ STATIC void
 xfs_cleanup_inode(
 	struct inode	*dir,
 	struct inode	*inode,
-	struct dentry	*dentry,
-	int		mode)
+	struct dentry	*dentry)
 {
 	struct xfs_name	teardown;
 
@@ -264,10 +263,7 @@ xfs_cleanup_inode(
 	 */
 	xfs_dentry_to_name(&teardown, dentry);
 
-	if (S_ISDIR(mode))
-		xfs_rmdir(XFS_I(dir), &teardown, XFS_I(inode));
-	else
-		xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
+	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
 	iput(inode);
 }
 
@@ -349,7 +345,7 @@ xfs_vn_mknod(
 	return -error;
 
  out_cleanup_inode:
-	xfs_cleanup_inode(dir, inode, dentry, mode);
+	xfs_cleanup_inode(dir, inode, dentry);
  out_free_acl:
 	if (default_acl)
 		_ACL_FREE(default_acl);
@@ -525,38 +521,12 @@ xfs_vn_symlink(
 	return 0;
 
  out_cleanup_inode:
-	xfs_cleanup_inode(dir, inode, dentry, 0);
+	xfs_cleanup_inode(dir, inode, dentry);
  out:
 	return -error;
 }
 
 STATIC int
-xfs_vn_rmdir(
-	struct inode	*dir,
-	struct dentry	*dentry)
-{
-	struct inode	*inode = dentry->d_inode;
-	struct xfs_name	name;
-	int		error;
-
-	xfs_dentry_to_name(&name, dentry);
-
-	error = xfs_rmdir(XFS_I(dir), &name, XFS_I(inode));
-	if (likely(!error)) {
-		xfs_validate_fields(inode);
-		xfs_validate_fields(dir);
-		/*
-		 * With rmdir, the VFS makes the dentry "negative": no inode,
-		 * but still hashed. This is incompatible with case-insensitive
-		 * mode, so invalidate (unhash) the dentry in CI-mode.
-		 */
-		if (xfs_sb_version_hasasciici(&XFS_M(dir->i_sb)->m_sb))
-			d_invalidate(dentry);
-	}
-	return -error;
-}
-
-STATIC int
 xfs_vn_rename(
 	struct inode	*odir,
 	struct dentry	*odentry,
@@ -850,7 +820,13 @@ static const struct inode_operations xfs
 	.unlink			= xfs_vn_unlink,
 	.symlink		= xfs_vn_symlink,
 	.mkdir			= xfs_vn_mkdir,
-	.rmdir			= xfs_vn_rmdir,
+	/*
+	 * Yes, XFS uses the same method for rmdir and unlink.
+	 *
+	 * There are some subtile differences deeper in the code,
+	 * but we use S_ISDIR to check for those.
+	 */
+	.rmdir			= xfs_vn_unlink,
 	.mknod			= xfs_vn_mknod,
 	.rename			= xfs_vn_rename,
 	.permission		= xfs_vn_permission,
@@ -869,7 +845,13 @@ static const struct inode_operations xfs
 	.unlink			= xfs_vn_unlink,
 	.symlink		= xfs_vn_symlink,
 	.mkdir			= xfs_vn_mkdir,
-	.rmdir			= xfs_vn_rmdir,
+	/*
+	 * Yes, XFS uses the same method for rmdir and unlink.
+	 *
+	 * There are some subtile differences deeper in the code,
+	 * but we use S_ISDIR to check for those.
+	 */
+	.rmdir			= xfs_vn_unlink,
 	.mknod			= xfs_vn_mknod,
 	.rename			= xfs_vn_rename,
 	.permission		= xfs_vn_permission,
Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.h
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.h	2008-06-16 08:11:12.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.h	2008-06-16 08:14:16.000000000 +0200
@@ -31,8 +31,6 @@ int xfs_link(struct xfs_inode *tdp, stru
 		struct xfs_name *target_name);
 int xfs_mkdir(struct xfs_inode *dp, struct xfs_name *dir_name,
 		mode_t mode, struct xfs_inode **ipp, struct cred *credp);
-int xfs_rmdir(struct xfs_inode *dp, struct xfs_name *name,
-		struct xfs_inode *cdp);
 int xfs_readdir(struct xfs_inode	*dp, void *dirent, size_t bufsize,
 		       xfs_off_t *offset, filldir_t filldir);
 int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,

Re: [PATCH 1/2] merge xfs_rmdir into xfs_remove

[Christoph Hellwig]

ping?

On Mon, Jun 16, 2008 at 08:18:39AM +0200, Christoph Hellwig wrote:
> On Wed, May 21, 2008 at 10:25:48AM +0200, Christoph Hellwig wrote:
> > On Fri, May 02, 2008 at 12:57:57PM +0200, Christoph Hellwig wrote:
> > > xfs_remove and xfs_rmdir are almost the same with a little more work
> > > performed in xfs_rmdir due to the . and .. entries.  This patch merges
> > > xfs_rmdir into xfs_remove and performs these actions conditionally.
> > > 
> > > Also clean up the error handling which was a nightmare in both versions
> > > before.
> > 
> > Updated for the case-insensitive filename changes.
> 
> And another update for the d_invalidate added on unlink/rmdir.
> 
> Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-06-16 08:11:12.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-06-16 08:14:16.000000000 +0200
> @@ -2116,13 +2116,6 @@ again:
>  #endif
>  }
>  
> -#ifdef	DEBUG
> -#define	REMOVE_DEBUG_TRACE(x)	{remove_which_error_return = (x);}
> -int remove_which_error_return = 0;
> -#else /* ! DEBUG */
> -#define	REMOVE_DEBUG_TRACE(x)
> -#endif	/* ! DEBUG */
> -
>  int
>  xfs_remove(
>  	xfs_inode_t             *dp,
> @@ -2131,6 +2124,7 @@ xfs_remove(
>  {
>  	xfs_mount_t		*mp = dp->i_mount;
>  	xfs_trans_t             *tp = NULL;
> +	int			is_dir = S_ISDIR(ip->i_d.di_mode);
>  	int                     error = 0;
>  	xfs_bmap_free_t         free_list;
>  	xfs_fsblock_t           first_block;
> @@ -2138,8 +2132,11 @@ xfs_remove(
>  	int			committed;
>  	int			link_zero;
>  	uint			resblks;
> +	uint			trans;
> +	uint			log_count;
>  
>  	xfs_itrace_entry(dp);
> +	xfs_itrace_entry(ip);
>  
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return XFS_ERROR(EIO);
> @@ -2152,19 +2149,25 @@ xfs_remove(
>  			return error;
>  	}
>  
> -	xfs_itrace_entry(ip);
> -	xfs_itrace_ref(ip);
> -
>  	error = XFS_QM_DQATTACH(mp, dp, 0);
> -	if (!error)
> -		error = XFS_QM_DQATTACH(mp, ip, 0);
> -	if (error) {
> -		REMOVE_DEBUG_TRACE(__LINE__);
> +	if (error)
> +		goto std_return;
> +
> +	error = XFS_QM_DQATTACH(mp, ip, 0);
> +	if (error)
>  		goto std_return;
> +
> +	if (is_dir) {
> +		trans = XFS_TRANS_RMDIR;
> +		log_count = XFS_DEFAULT_LOG_COUNT;
> +	} else {
> +		trans = XFS_TRANS_REMOVE;
> +		log_count = XFS_REMOVE_LOG_COUNT;
>  	}
>  
> -	tp = xfs_trans_alloc(mp, XFS_TRANS_REMOVE);
> +	tp = xfs_trans_alloc(mp, trans);
>  	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> +
>  	/*
>  	 * We try to get the real space reservation first,
>  	 * allowing for directory btree deletion(s) implying
> @@ -2176,25 +2179,21 @@ xfs_remove(
>  	 */
>  	resblks = XFS_REMOVE_SPACE_RES(mp);
>  	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
> -			XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
> +				  XFS_TRANS_PERM_LOG_RES, log_count);
>  	if (error == ENOSPC) {
>  		resblks = 0;
>  		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
> -				XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
> +					  XFS_TRANS_PERM_LOG_RES, log_count);
>  	}
>  	if (error) {
>  		ASSERT(error != ENOSPC);
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		xfs_trans_cancel(tp, 0);
> -		return error;
> +		cancel_flags = 0;
> +		goto out_trans_cancel;
>  	}
>  
>  	error = xfs_lock_dir_and_entry(dp, ip);
> -	if (error) {
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		xfs_trans_cancel(tp, cancel_flags);
> -		goto std_return;
> -	}
> +	if (error)
> +		goto out_trans_cancel;
>  
>  	/*
>  	 * At this point, we've gotten both the directory and the entry
> @@ -2207,6 +2206,21 @@ xfs_remove(
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  
>  	/*
> +	 * If we're removing a directory perform some additional validation.
> +	 */
> +	if (is_dir) {
> +		ASSERT(ip->i_d.di_nlink >= 2);
> +		if (ip->i_d.di_nlink != 2) {
> +			error = XFS_ERROR(ENOTEMPTY);
> +			goto out_trans_cancel;
> +		}
> +		if (!xfs_dir_isempty(ip)) {
> +			error = XFS_ERROR(ENOTEMPTY);
> +			goto out_trans_cancel;
> +		}
> +	}
> +
> +	/*
>  	 * Entry must exist since we did a lookup in xfs_lock_dir_and_entry.
>  	 */
>  	XFS_BMAP_INIT(&free_list, &first_block);
> @@ -2214,39 +2228,64 @@ xfs_remove(
>  					&first_block, &free_list, resblks);
>  	if (error) {
>  		ASSERT(error != ENOENT);
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		goto error1;
> +		goto out_bmap_cancel;
>  	}
>  	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
>  
> +	/*
> +	 * Bump the in memory generation count on the parent
> +	 * directory so that other can know that it has changed.
> +	 */
>  	dp->i_gen++;
>  	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
>  
> -	error = xfs_droplink(tp, ip);
> -	if (error) {
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		goto error1;
> +	if (is_dir) {
> +		/*
> +		 * Drop the link from ip's "..".
> +		 */
> +		error = xfs_droplink(tp, dp);
> +		if (error)
> +			goto out_bmap_cancel;
> +
> +		/*
> +		 * Drop the link from dp to ip.
> +		 */
> +		error = xfs_droplink(tp, ip);
> +		if (error)
> +			goto out_bmap_cancel;
> +	} else {
> +		/*
> +		 * When removing a non-directory we need to log the parent
> +		 * inode here for the i_gen update.  For a directory this is
> +		 * done implicitly by the xfs_droplink call for the ".." entry.
> +		 */
> +		xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
>  	}
>  
> -	/* Determine if this is the last link while
> +	/*
> +	 * Drop the "." link from ip to self.
> +	 */
> +	error = xfs_droplink(tp, ip);
> +	if (error)
> +		goto out_bmap_cancel;
> +
> +	/*
> +	 * Determine if this is the last link while
>  	 * we are in the transaction.
>  	 */
> -	link_zero = (ip)->i_d.di_nlink==0;
> +	link_zero = (ip->i_d.di_nlink == 0);
>  
>  	/*
>  	 * If this is a synchronous mount, make sure that the
>  	 * remove transaction goes to disk before returning to
>  	 * the user.
>  	 */
> -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
> +	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
>  		xfs_trans_set_sync(tp);
> -	}
>  
>  	error = xfs_bmap_finish(&tp, &free_list, &committed);
> -	if (error) {
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		goto error_rele;
> -	}
> +	if (error)
> +		goto out_bmap_cancel;
>  
>  	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
>  	if (error)
> @@ -2258,39 +2297,27 @@ xfs_remove(
>  	 * will get killed on last close in xfs_close() so we don't
>  	 * have to worry about that.
>  	 */
> -	if (link_zero && xfs_inode_is_filestream(ip))
> +	if (!is_dir && link_zero && xfs_inode_is_filestream(ip))
>  		xfs_filestream_deassociate(ip);
>  
>  	xfs_itrace_exit(ip);
> +	xfs_itrace_exit(dp);
>  
> -/*	Fall through to std_return with error = 0 */
>   std_return:
>  	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
> -		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
> -				dp, DM_RIGHT_NULL,
> -				NULL, DM_RIGHT_NULL,
> -				name->name, NULL, ip->i_d.di_mode, error, 0);
> +		XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE, dp, DM_RIGHT_NULL,
> +				NULL, DM_RIGHT_NULL, name->name, NULL,
> +				ip->i_d.di_mode, error, 0);
>  	}
> +
>  	return error;
>  
> - error1:
> + out_bmap_cancel:
>  	xfs_bmap_cancel(&free_list);
>  	cancel_flags |= XFS_TRANS_ABORT;
> + out_trans_cancel:
>  	xfs_trans_cancel(tp, cancel_flags);
>  	goto std_return;
> -
> - error_rele:
> -	/*
> -	 * In this case make sure to not release the inode until after
> -	 * the current transaction is aborted.  Releasing it beforehand
> -	 * can cause us to go to xfs_inactive and start a recursive
> -	 * transaction which can easily deadlock with the current one.
> -	 */
> -	xfs_bmap_cancel(&free_list);
> -	cancel_flags |= XFS_TRANS_ABORT;
> -	xfs_trans_cancel(tp, cancel_flags);
> -
> -	goto std_return;
>  }
>  
>  int
> @@ -2656,186 +2683,6 @@ std_return:
>  }
>  
>  int
> -xfs_rmdir(
> -	xfs_inode_t             *dp,
> -	struct xfs_name		*name,
> -	xfs_inode_t		*cdp)
> -{
> -	xfs_mount_t		*mp = dp->i_mount;
> -	xfs_trans_t             *tp;
> -	int                     error;
> -	xfs_bmap_free_t         free_list;
> -	xfs_fsblock_t           first_block;
> -	int			cancel_flags;
> -	int			committed;
> -	int			last_cdp_link;
> -	uint			resblks;
> -
> -	xfs_itrace_entry(dp);
> -
> -	if (XFS_FORCED_SHUTDOWN(mp))
> -		return XFS_ERROR(EIO);
> -
> -	if (DM_EVENT_ENABLED(dp, DM_EVENT_REMOVE)) {
> -		error = XFS_SEND_NAMESP(mp, DM_EVENT_REMOVE,
> -					dp, DM_RIGHT_NULL,
> -					NULL, DM_RIGHT_NULL, name->name,
> -					NULL, cdp->i_d.di_mode, 0, 0);
> -		if (error)
> -			return XFS_ERROR(error);
> -	}
> -
> -	/*
> -	 * Get the dquots for the inodes.
> -	 */
> -	error = XFS_QM_DQATTACH(mp, dp, 0);
> -	if (!error)
> -		error = XFS_QM_DQATTACH(mp, cdp, 0);
> -	if (error) {
> -		REMOVE_DEBUG_TRACE(__LINE__);
> -		goto std_return;
> -	}
> -
> -	tp = xfs_trans_alloc(mp, XFS_TRANS_RMDIR);
> -	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> -	/*
> -	 * We try to get the real space reservation first,
> -	 * allowing for directory btree deletion(s) implying
> -	 * possible bmap insert(s).  If we can't get the space
> -	 * reservation then we use 0 instead, and avoid the bmap
> -	 * btree insert(s) in the directory code by, if the bmap
> -	 * insert tries to happen, instead trimming the LAST
> -	 * block from the directory.
> -	 */
> -	resblks = XFS_REMOVE_SPACE_RES(mp);
> -	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
> -			XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
> -	if (error == ENOSPC) {
> -		resblks = 0;
> -		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
> -				XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
> -	}
> -	if (error) {
> -		ASSERT(error != ENOSPC);
> -		cancel_flags = 0;
> -		goto error_return;
> -	}
> -	XFS_BMAP_INIT(&free_list, &first_block);
> -
> -	/*
> -	 * Now lock the child directory inode and the parent directory
> -	 * inode in the proper order.  This will take care of validating
> -	 * that the directory entry for the child directory inode has
> -	 * not changed while we were obtaining a log reservation.
> -	 */
> -	error = xfs_lock_dir_and_entry(dp, cdp);
> -	if (error) {
> -		xfs_trans_cancel(tp, cancel_flags);
> -		goto std_return;
> -	}
> -
> -	IHOLD(dp);
> -	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
> -
> -	IHOLD(cdp);
> -	xfs_trans_ijoin(tp, cdp, XFS_ILOCK_EXCL);
> -
> -	ASSERT(cdp->i_d.di_nlink >= 2);
> -	if (cdp->i_d.di_nlink != 2) {
> -		error = XFS_ERROR(ENOTEMPTY);
> -		goto error_return;
> -	}
> -	if (!xfs_dir_isempty(cdp)) {
> -		error = XFS_ERROR(ENOTEMPTY);
> -		goto error_return;
> -	}
> -
> -	error = xfs_dir_removename(tp, dp, name, cdp->i_ino,
> -					&first_block, &free_list, resblks);
> -	if (error)
> -		goto error1;
> -
> -	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> -
> -	/*
> -	 * Bump the in memory generation count on the parent
> -	 * directory so that other can know that it has changed.
> -	 */
> -	dp->i_gen++;
> -
> -	/*
> -	 * Drop the link from cdp's "..".
> -	 */
> -	error = xfs_droplink(tp, dp);
> -	if (error) {
> -		goto error1;
> -	}
> -
> -	/*
> -	 * Drop the link from dp to cdp.
> -	 */
> -	error = xfs_droplink(tp, cdp);
> -	if (error) {
> -		goto error1;
> -	}
> -
> -	/*
> -	 * Drop the "." link from cdp to self.
> -	 */
> -	error = xfs_droplink(tp, cdp);
> -	if (error) {
> -		goto error1;
> -	}
> -
> -	/* Determine these before committing transaction */
> -	last_cdp_link = (cdp)->i_d.di_nlink==0;
> -
> -	/*
> -	 * If this is a synchronous mount, make sure that the
> -	 * rmdir transaction goes to disk before returning to
> -	 * the user.
> -	 */
> -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
> -		xfs_trans_set_sync(tp);
> -	}
> -
> -	error = xfs_bmap_finish (&tp, &free_list, &committed);
> -	if (error) {
> -		xfs_bmap_cancel(&free_list);
> -		xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES |
> -				 XFS_TRANS_ABORT));
> -		goto std_return;
> -	}
> -
> -	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> -	if (error) {
> -		goto std_return;
> -	}
> -
> -
> -	/* Fall through to std_return with error = 0 or the errno
> -	 * from xfs_trans_commit. */
> - std_return:
> -	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
> -		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
> -					dp, DM_RIGHT_NULL,
> -					NULL, DM_RIGHT_NULL,
> -					name->name, NULL, cdp->i_d.di_mode,
> -					error, 0);
> -	}
> -	return error;
> -
> - error1:
> -	xfs_bmap_cancel(&free_list);
> -	cancel_flags |= XFS_TRANS_ABORT;
> -	/* FALLTHROUGH */
> -
> - error_return:
> -	xfs_trans_cancel(tp, cancel_flags);
> -	goto std_return;
> -}
> -
> -int
>  xfs_symlink(
>  	xfs_inode_t		*dp,
>  	struct xfs_name		*link_name,
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-06-16 08:14:12.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c	2008-06-16 08:15:00.000000000 +0200
> @@ -252,8 +252,7 @@ STATIC void
>  xfs_cleanup_inode(
>  	struct inode	*dir,
>  	struct inode	*inode,
> -	struct dentry	*dentry,
> -	int		mode)
> +	struct dentry	*dentry)
>  {
>  	struct xfs_name	teardown;
>  
> @@ -264,10 +263,7 @@ xfs_cleanup_inode(
>  	 */
>  	xfs_dentry_to_name(&teardown, dentry);
>  
> -	if (S_ISDIR(mode))
> -		xfs_rmdir(XFS_I(dir), &teardown, XFS_I(inode));
> -	else
> -		xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
> +	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
>  	iput(inode);
>  }
>  
> @@ -349,7 +345,7 @@ xfs_vn_mknod(
>  	return -error;
>  
>   out_cleanup_inode:
> -	xfs_cleanup_inode(dir, inode, dentry, mode);
> +	xfs_cleanup_inode(dir, inode, dentry);
>   out_free_acl:
>  	if (default_acl)
>  		_ACL_FREE(default_acl);
> @@ -525,38 +521,12 @@ xfs_vn_symlink(
>  	return 0;
>  
>   out_cleanup_inode:
> -	xfs_cleanup_inode(dir, inode, dentry, 0);
> +	xfs_cleanup_inode(dir, inode, dentry);
>   out:
>  	return -error;
>  }
>  
>  STATIC int
> -xfs_vn_rmdir(
> -	struct inode	*dir,
> -	struct dentry	*dentry)
> -{
> -	struct inode	*inode = dentry->d_inode;
> -	struct xfs_name	name;
> -	int		error;
> -
> -	xfs_dentry_to_name(&name, dentry);
> -
> -	error = xfs_rmdir(XFS_I(dir), &name, XFS_I(inode));
> -	if (likely(!error)) {
> -		xfs_validate_fields(inode);
> -		xfs_validate_fields(dir);
> -		/*
> -		 * With rmdir, the VFS makes the dentry "negative": no inode,
> -		 * but still hashed. This is incompatible with case-insensitive
> -		 * mode, so invalidate (unhash) the dentry in CI-mode.
> -		 */
> -		if (xfs_sb_version_hasasciici(&XFS_M(dir->i_sb)->m_sb))
> -			d_invalidate(dentry);
> -	}
> -	return -error;
> -}
> -
> -STATIC int
>  xfs_vn_rename(
>  	struct inode	*odir,
>  	struct dentry	*odentry,
> @@ -850,7 +820,13 @@ static const struct inode_operations xfs
>  	.unlink			= xfs_vn_unlink,
>  	.symlink		= xfs_vn_symlink,
>  	.mkdir			= xfs_vn_mkdir,
> -	.rmdir			= xfs_vn_rmdir,
> +	/*
> +	 * Yes, XFS uses the same method for rmdir and unlink.
> +	 *
> +	 * There are some subtile differences deeper in the code,
> +	 * but we use S_ISDIR to check for those.
> +	 */
> +	.rmdir			= xfs_vn_unlink,
>  	.mknod			= xfs_vn_mknod,
>  	.rename			= xfs_vn_rename,
>  	.permission		= xfs_vn_permission,
> @@ -869,7 +845,13 @@ static const struct inode_operations xfs
>  	.unlink			= xfs_vn_unlink,
>  	.symlink		= xfs_vn_symlink,
>  	.mkdir			= xfs_vn_mkdir,
> -	.rmdir			= xfs_vn_rmdir,
> +	/*
> +	 * Yes, XFS uses the same method for rmdir and unlink.
> +	 *
> +	 * There are some subtile differences deeper in the code,
> +	 * but we use S_ISDIR to check for those.
> +	 */
> +	.rmdir			= xfs_vn_unlink,
>  	.mknod			= xfs_vn_mknod,
>  	.rename			= xfs_vn_rename,
>  	.permission		= xfs_vn_permission,
> Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.h	2008-06-16 08:11:12.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.h	2008-06-16 08:14:16.000000000 +0200
> @@ -31,8 +31,6 @@ int xfs_link(struct xfs_inode *tdp, stru
>  		struct xfs_name *target_name);
>  int xfs_mkdir(struct xfs_inode *dp, struct xfs_name *dir_name,
>  		mode_t mode, struct xfs_inode **ipp, struct cred *credp);
> -int xfs_rmdir(struct xfs_inode *dp, struct xfs_name *name,
> -		struct xfs_inode *cdp);
>  int xfs_readdir(struct xfs_inode	*dp, void *dirent, size_t bufsize,
>  		       xfs_off_t *offset, filldir_t filldir);
>  int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,
---end quoted text---

Re: [PATCH 1/2] merge xfs_rmdir into xfs_remove

[Christoph Hellwig]

Oops, sorry.  This one obviously is in already, I men to ping from 2/2
from this series.

On Wed, Jul 23, 2008 at 10:06:18AM +0200, Christoph Hellwig wrote:
> ping?
> 
> On Mon, Jun 16, 2008 at 08:18:39AM +0200, Christoph Hellwig wrote:
> > On Wed, May 21, 2008 at 10:25:48AM +0200, Christoph Hellwig wrote:
> > > On Fri, May 02, 2008 at 12:57:57PM +0200, Christoph Hellwig wrote:
> > > > xfs_remove and xfs_rmdir are almost the same with a little more work
> > > > performed in xfs_rmdir due to the . and .. entries.  This patch merges
> > > > xfs_rmdir into xfs_remove and performs these actions conditionally.
> > > > 
> > > > Also clean up the error handling which was a nightmare in both versions
> > > > before.
> > > 
> > > Updated for the case-insensitive filename changes.
> > 
> > And another update for the d_invalidate added on unlink/rmdir.
> > 
> > Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
> > ===================================================================
> > --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-06-16 08:11:12.000000000 +0200
> > +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-06-16 08:14:16.000000000 +0200
> > @@ -2116,13 +2116,6 @@ again:
> >  #endif
> >  }
> >  
> > -#ifdef	DEBUG
> > -#define	REMOVE_DEBUG_TRACE(x)	{remove_which_error_return = (x);}
> > -int remove_which_error_return = 0;
> > -#else /* ! DEBUG */
> > -#define	REMOVE_DEBUG_TRACE(x)
> > -#endif	/* ! DEBUG */
> > -
> >  int
> >  xfs_remove(
> >  	xfs_inode_t             *dp,
> > @@ -2131,6 +2124,7 @@ xfs_remove(
> >  {
> >  	xfs_mount_t		*mp = dp->i_mount;
> >  	xfs_trans_t             *tp = NULL;
> > +	int			is_dir = S_ISDIR(ip->i_d.di_mode);
> >  	int                     error = 0;
> >  	xfs_bmap_free_t         free_list;
> >  	xfs_fsblock_t           first_block;
> > @@ -2138,8 +2132,11 @@ xfs_remove(
> >  	int			committed;
> >  	int			link_zero;
> >  	uint			resblks;
> > +	uint			trans;
> > +	uint			log_count;
> >  
> >  	xfs_itrace_entry(dp);
> > +	xfs_itrace_entry(ip);
> >  
> >  	if (XFS_FORCED_SHUTDOWN(mp))
> >  		return XFS_ERROR(EIO);
> > @@ -2152,19 +2149,25 @@ xfs_remove(
> >  			return error;
> >  	}
> >  
> > -	xfs_itrace_entry(ip);
> > -	xfs_itrace_ref(ip);
> > -
> >  	error = XFS_QM_DQATTACH(mp, dp, 0);
> > -	if (!error)
> > -		error = XFS_QM_DQATTACH(mp, ip, 0);
> > -	if (error) {
> > -		REMOVE_DEBUG_TRACE(__LINE__);
> > +	if (error)
> > +		goto std_return;
> > +
> > +	error = XFS_QM_DQATTACH(mp, ip, 0);
> > +	if (error)
> >  		goto std_return;
> > +
> > +	if (is_dir) {
> > +		trans = XFS_TRANS_RMDIR;
> > +		log_count = XFS_DEFAULT_LOG_COUNT;
> > +	} else {
> > +		trans = XFS_TRANS_REMOVE;
> > +		log_count = XFS_REMOVE_LOG_COUNT;
> >  	}
> >  
> > -	tp = xfs_trans_alloc(mp, XFS_TRANS_REMOVE);
> > +	tp = xfs_trans_alloc(mp, trans);
> >  	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> > +
> >  	/*
> >  	 * We try to get the real space reservation first,
> >  	 * allowing for directory btree deletion(s) implying
> > @@ -2176,25 +2179,21 @@ xfs_remove(
> >  	 */
> >  	resblks = XFS_REMOVE_SPACE_RES(mp);
> >  	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
> > -			XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
> > +				  XFS_TRANS_PERM_LOG_RES, log_count);
> >  	if (error == ENOSPC) {
> >  		resblks = 0;
> >  		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
> > -				XFS_TRANS_PERM_LOG_RES, XFS_REMOVE_LOG_COUNT);
> > +					  XFS_TRANS_PERM_LOG_RES, log_count);
> >  	}
> >  	if (error) {
> >  		ASSERT(error != ENOSPC);
> > -		REMOVE_DEBUG_TRACE(__LINE__);
> > -		xfs_trans_cancel(tp, 0);
> > -		return error;
> > +		cancel_flags = 0;
> > +		goto out_trans_cancel;
> >  	}
> >  
> >  	error = xfs_lock_dir_and_entry(dp, ip);
> > -	if (error) {
> > -		REMOVE_DEBUG_TRACE(__LINE__);
> > -		xfs_trans_cancel(tp, cancel_flags);
> > -		goto std_return;
> > -	}
> > +	if (error)
> > +		goto out_trans_cancel;
> >  
> >  	/*
> >  	 * At this point, we've gotten both the directory and the entry
> > @@ -2207,6 +2206,21 @@ xfs_remove(
> >  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> >  
> >  	/*
> > +	 * If we're removing a directory perform some additional validation.
> > +	 */
> > +	if (is_dir) {
> > +		ASSERT(ip->i_d.di_nlink >= 2);
> > +		if (ip->i_d.di_nlink != 2) {
> > +			error = XFS_ERROR(ENOTEMPTY);
> > +			goto out_trans_cancel;
> > +		}
> > +		if (!xfs_dir_isempty(ip)) {
> > +			error = XFS_ERROR(ENOTEMPTY);
> > +			goto out_trans_cancel;
> > +		}
> > +	}
> > +
> > +	/*
> >  	 * Entry must exist since we did a lookup in xfs_lock_dir_and_entry.
> >  	 */
> >  	XFS_BMAP_INIT(&free_list, &first_block);
> > @@ -2214,39 +2228,64 @@ xfs_remove(
> >  					&first_block, &free_list, resblks);
> >  	if (error) {
> >  		ASSERT(error != ENOENT);
> > -		REMOVE_DEBUG_TRACE(__LINE__);
> > -		goto error1;
> > +		goto out_bmap_cancel;
> >  	}
> >  	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >  
> > +	/*
> > +	 * Bump the in memory generation count on the parent
> > +	 * directory so that other can know that it has changed.
> > +	 */
> >  	dp->i_gen++;
> >  	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
> >  
> > -	error = xfs_droplink(tp, ip);
> > -	if (error) {
> > -		REMOVE_DEBUG_TRACE(__LINE__);
> > -		goto error1;
> > +	if (is_dir) {
> > +		/*
> > +		 * Drop the link from ip's "..".
> > +		 */
> > +		error = xfs_droplink(tp, dp);
> > +		if (error)
> > +			goto out_bmap_cancel;
> > +
> > +		/*
> > +		 * Drop the link from dp to ip.
> > +		 */
> > +		error = xfs_droplink(tp, ip);
> > +		if (error)
> > +			goto out_bmap_cancel;
> > +	} else {
> > +		/*
> > +		 * When removing a non-directory we need to log the parent
> > +		 * inode here for the i_gen update.  For a directory this is
> > +		 * done implicitly by the xfs_droplink call for the ".." entry.
> > +		 */
> > +		xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
> >  	}
> >  
> > -	/* Determine if this is the last link while
> > +	/*
> > +	 * Drop the "." link from ip to self.
> > +	 */
> > +	error = xfs_droplink(tp, ip);
> > +	if (error)
> > +		goto out_bmap_cancel;
> > +
> > +	/*
> > +	 * Determine if this is the last link while
> >  	 * we are in the transaction.
> >  	 */
> > -	link_zero = (ip)->i_d.di_nlink==0;
> > +	link_zero = (ip->i_d.di_nlink == 0);
> >  
> >  	/*
> >  	 * If this is a synchronous mount, make sure that the
> >  	 * remove transaction goes to disk before returning to
> >  	 * the user.
> >  	 */
> > -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
> > +	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> >  		xfs_trans_set_sync(tp);
> > -	}
> >  
> >  	error = xfs_bmap_finish(&tp, &free_list, &committed);
> > -	if (error) {
> > -		REMOVE_DEBUG_TRACE(__LINE__);
> > -		goto error_rele;
> > -	}
> > +	if (error)
> > +		goto out_bmap_cancel;
> >  
> >  	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> >  	if (error)
> > @@ -2258,39 +2297,27 @@ xfs_remove(
> >  	 * will get killed on last close in xfs_close() so we don't
> >  	 * have to worry about that.
> >  	 */
> > -	if (link_zero && xfs_inode_is_filestream(ip))
> > +	if (!is_dir && link_zero && xfs_inode_is_filestream(ip))
> >  		xfs_filestream_deassociate(ip);
> >  
> >  	xfs_itrace_exit(ip);
> > +	xfs_itrace_exit(dp);
> >  
> > -/*	Fall through to std_return with error = 0 */
> >   std_return:
> >  	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
> > -		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
> > -				dp, DM_RIGHT_NULL,
> > -				NULL, DM_RIGHT_NULL,
> > -				name->name, NULL, ip->i_d.di_mode, error, 0);
> > +		XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE, dp, DM_RIGHT_NULL,
> > +				NULL, DM_RIGHT_NULL, name->name, NULL,
> > +				ip->i_d.di_mode, error, 0);
> >  	}
> > +
> >  	return error;
> >  
> > - error1:
> > + out_bmap_cancel:
> >  	xfs_bmap_cancel(&free_list);
> >  	cancel_flags |= XFS_TRANS_ABORT;
> > + out_trans_cancel:
> >  	xfs_trans_cancel(tp, cancel_flags);
> >  	goto std_return;
> > -
> > - error_rele:
> > -	/*
> > -	 * In this case make sure to not release the inode until after
> > -	 * the current transaction is aborted.  Releasing it beforehand
> > -	 * can cause us to go to xfs_inactive and start a recursive
> > -	 * transaction which can easily deadlock with the current one.
> > -	 */
> > -	xfs_bmap_cancel(&free_list);
> > -	cancel_flags |= XFS_TRANS_ABORT;
> > -	xfs_trans_cancel(tp, cancel_flags);
> > -
> > -	goto std_return;
> >  }
> >  
> >  int
> > @@ -2656,186 +2683,6 @@ std_return:
> >  }
> >  
> >  int
> > -xfs_rmdir(
> > -	xfs_inode_t             *dp,
> > -	struct xfs_name		*name,
> > -	xfs_inode_t		*cdp)
> > -{
> > -	xfs_mount_t		*mp = dp->i_mount;
> > -	xfs_trans_t             *tp;
> > -	int                     error;
> > -	xfs_bmap_free_t         free_list;
> > -	xfs_fsblock_t           first_block;
> > -	int			cancel_flags;
> > -	int			committed;
> > -	int			last_cdp_link;
> > -	uint			resblks;
> > -
> > -	xfs_itrace_entry(dp);
> > -
> > -	if (XFS_FORCED_SHUTDOWN(mp))
> > -		return XFS_ERROR(EIO);
> > -
> > -	if (DM_EVENT_ENABLED(dp, DM_EVENT_REMOVE)) {
> > -		error = XFS_SEND_NAMESP(mp, DM_EVENT_REMOVE,
> > -					dp, DM_RIGHT_NULL,
> > -					NULL, DM_RIGHT_NULL, name->name,
> > -					NULL, cdp->i_d.di_mode, 0, 0);
> > -		if (error)
> > -			return XFS_ERROR(error);
> > -	}
> > -
> > -	/*
> > -	 * Get the dquots for the inodes.
> > -	 */
> > -	error = XFS_QM_DQATTACH(mp, dp, 0);
> > -	if (!error)
> > -		error = XFS_QM_DQATTACH(mp, cdp, 0);
> > -	if (error) {
> > -		REMOVE_DEBUG_TRACE(__LINE__);
> > -		goto std_return;
> > -	}
> > -
> > -	tp = xfs_trans_alloc(mp, XFS_TRANS_RMDIR);
> > -	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> > -	/*
> > -	 * We try to get the real space reservation first,
> > -	 * allowing for directory btree deletion(s) implying
> > -	 * possible bmap insert(s).  If we can't get the space
> > -	 * reservation then we use 0 instead, and avoid the bmap
> > -	 * btree insert(s) in the directory code by, if the bmap
> > -	 * insert tries to happen, instead trimming the LAST
> > -	 * block from the directory.
> > -	 */
> > -	resblks = XFS_REMOVE_SPACE_RES(mp);
> > -	error = xfs_trans_reserve(tp, resblks, XFS_REMOVE_LOG_RES(mp), 0,
> > -			XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
> > -	if (error == ENOSPC) {
> > -		resblks = 0;
> > -		error = xfs_trans_reserve(tp, 0, XFS_REMOVE_LOG_RES(mp), 0,
> > -				XFS_TRANS_PERM_LOG_RES, XFS_DEFAULT_LOG_COUNT);
> > -	}
> > -	if (error) {
> > -		ASSERT(error != ENOSPC);
> > -		cancel_flags = 0;
> > -		goto error_return;
> > -	}
> > -	XFS_BMAP_INIT(&free_list, &first_block);
> > -
> > -	/*
> > -	 * Now lock the child directory inode and the parent directory
> > -	 * inode in the proper order.  This will take care of validating
> > -	 * that the directory entry for the child directory inode has
> > -	 * not changed while we were obtaining a log reservation.
> > -	 */
> > -	error = xfs_lock_dir_and_entry(dp, cdp);
> > -	if (error) {
> > -		xfs_trans_cancel(tp, cancel_flags);
> > -		goto std_return;
> > -	}
> > -
> > -	IHOLD(dp);
> > -	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
> > -
> > -	IHOLD(cdp);
> > -	xfs_trans_ijoin(tp, cdp, XFS_ILOCK_EXCL);
> > -
> > -	ASSERT(cdp->i_d.di_nlink >= 2);
> > -	if (cdp->i_d.di_nlink != 2) {
> > -		error = XFS_ERROR(ENOTEMPTY);
> > -		goto error_return;
> > -	}
> > -	if (!xfs_dir_isempty(cdp)) {
> > -		error = XFS_ERROR(ENOTEMPTY);
> > -		goto error_return;
> > -	}
> > -
> > -	error = xfs_dir_removename(tp, dp, name, cdp->i_ino,
> > -					&first_block, &free_list, resblks);
> > -	if (error)
> > -		goto error1;
> > -
> > -	xfs_ichgtime(dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> > -
> > -	/*
> > -	 * Bump the in memory generation count on the parent
> > -	 * directory so that other can know that it has changed.
> > -	 */
> > -	dp->i_gen++;
> > -
> > -	/*
> > -	 * Drop the link from cdp's "..".
> > -	 */
> > -	error = xfs_droplink(tp, dp);
> > -	if (error) {
> > -		goto error1;
> > -	}
> > -
> > -	/*
> > -	 * Drop the link from dp to cdp.
> > -	 */
> > -	error = xfs_droplink(tp, cdp);
> > -	if (error) {
> > -		goto error1;
> > -	}
> > -
> > -	/*
> > -	 * Drop the "." link from cdp to self.
> > -	 */
> > -	error = xfs_droplink(tp, cdp);
> > -	if (error) {
> > -		goto error1;
> > -	}
> > -
> > -	/* Determine these before committing transaction */
> > -	last_cdp_link = (cdp)->i_d.di_nlink==0;
> > -
> > -	/*
> > -	 * If this is a synchronous mount, make sure that the
> > -	 * rmdir transaction goes to disk before returning to
> > -	 * the user.
> > -	 */
> > -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
> > -		xfs_trans_set_sync(tp);
> > -	}
> > -
> > -	error = xfs_bmap_finish (&tp, &free_list, &committed);
> > -	if (error) {
> > -		xfs_bmap_cancel(&free_list);
> > -		xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES |
> > -				 XFS_TRANS_ABORT));
> > -		goto std_return;
> > -	}
> > -
> > -	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> > -	if (error) {
> > -		goto std_return;
> > -	}
> > -
> > -
> > -	/* Fall through to std_return with error = 0 or the errno
> > -	 * from xfs_trans_commit. */
> > - std_return:
> > -	if (DM_EVENT_ENABLED(dp, DM_EVENT_POSTREMOVE)) {
> > -		(void) XFS_SEND_NAMESP(mp, DM_EVENT_POSTREMOVE,
> > -					dp, DM_RIGHT_NULL,
> > -					NULL, DM_RIGHT_NULL,
> > -					name->name, NULL, cdp->i_d.di_mode,
> > -					error, 0);
> > -	}
> > -	return error;
> > -
> > - error1:
> > -	xfs_bmap_cancel(&free_list);
> > -	cancel_flags |= XFS_TRANS_ABORT;
> > -	/* FALLTHROUGH */
> > -
> > - error_return:
> > -	xfs_trans_cancel(tp, cancel_flags);
> > -	goto std_return;
> > -}
> > -
> > -int
> >  xfs_symlink(
> >  	xfs_inode_t		*dp,
> >  	struct xfs_name		*link_name,
> > Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
> > ===================================================================
> > --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-06-16 08:14:12.000000000 +0200
> > +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c	2008-06-16 08:15:00.000000000 +0200
> > @@ -252,8 +252,7 @@ STATIC void
> >  xfs_cleanup_inode(
> >  	struct inode	*dir,
> >  	struct inode	*inode,
> > -	struct dentry	*dentry,
> > -	int		mode)
> > +	struct dentry	*dentry)
> >  {
> >  	struct xfs_name	teardown;
> >  
> > @@ -264,10 +263,7 @@ xfs_cleanup_inode(
> >  	 */
> >  	xfs_dentry_to_name(&teardown, dentry);
> >  
> > -	if (S_ISDIR(mode))
> > -		xfs_rmdir(XFS_I(dir), &teardown, XFS_I(inode));
> > -	else
> > -		xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
> > +	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
> >  	iput(inode);
> >  }
> >  
> > @@ -349,7 +345,7 @@ xfs_vn_mknod(
> >  	return -error;
> >  
> >   out_cleanup_inode:
> > -	xfs_cleanup_inode(dir, inode, dentry, mode);
> > +	xfs_cleanup_inode(dir, inode, dentry);
> >   out_free_acl:
> >  	if (default_acl)
> >  		_ACL_FREE(default_acl);
> > @@ -525,38 +521,12 @@ xfs_vn_symlink(
> >  	return 0;
> >  
> >   out_cleanup_inode:
> > -	xfs_cleanup_inode(dir, inode, dentry, 0);
> > +	xfs_cleanup_inode(dir, inode, dentry);
> >   out:
> >  	return -error;
> >  }
> >  
> >  STATIC int
> > -xfs_vn_rmdir(
> > -	struct inode	*dir,
> > -	struct dentry	*dentry)
> > -{
> > -	struct inode	*inode = dentry->d_inode;
> > -	struct xfs_name	name;
> > -	int		error;
> > -
> > -	xfs_dentry_to_name(&name, dentry);
> > -
> > -	error = xfs_rmdir(XFS_I(dir), &name, XFS_I(inode));
> > -	if (likely(!error)) {
> > -		xfs_validate_fields(inode);
> > -		xfs_validate_fields(dir);
> > -		/*
> > -		 * With rmdir, the VFS makes the dentry "negative": no inode,
> > -		 * but still hashed. This is incompatible with case-insensitive
> > -		 * mode, so invalidate (unhash) the dentry in CI-mode.
> > -		 */
> > -		if (xfs_sb_version_hasasciici(&XFS_M(dir->i_sb)->m_sb))
> > -			d_invalidate(dentry);
> > -	}
> > -	return -error;
> > -}
> > -
> > -STATIC int
> >  xfs_vn_rename(
> >  	struct inode	*odir,
> >  	struct dentry	*odentry,
> > @@ -850,7 +820,13 @@ static const struct inode_operations xfs
> >  	.unlink			= xfs_vn_unlink,
> >  	.symlink		= xfs_vn_symlink,
> >  	.mkdir			= xfs_vn_mkdir,
> > -	.rmdir			= xfs_vn_rmdir,
> > +	/*
> > +	 * Yes, XFS uses the same method for rmdir and unlink.
> > +	 *
> > +	 * There are some subtile differences deeper in the code,
> > +	 * but we use S_ISDIR to check for those.
> > +	 */
> > +	.rmdir			= xfs_vn_unlink,
> >  	.mknod			= xfs_vn_mknod,
> >  	.rename			= xfs_vn_rename,
> >  	.permission		= xfs_vn_permission,
> > @@ -869,7 +845,13 @@ static const struct inode_operations xfs
> >  	.unlink			= xfs_vn_unlink,
> >  	.symlink		= xfs_vn_symlink,
> >  	.mkdir			= xfs_vn_mkdir,
> > -	.rmdir			= xfs_vn_rmdir,
> > +	/*
> > +	 * Yes, XFS uses the same method for rmdir and unlink.
> > +	 *
> > +	 * There are some subtile differences deeper in the code,
> > +	 * but we use S_ISDIR to check for those.
> > +	 */
> > +	.rmdir			= xfs_vn_unlink,
> >  	.mknod			= xfs_vn_mknod,
> >  	.rename			= xfs_vn_rename,
> >  	.permission		= xfs_vn_permission,
> > Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.h
> > ===================================================================
> > --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.h	2008-06-16 08:11:12.000000000 +0200
> > +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.h	2008-06-16 08:14:16.000000000 +0200
> > @@ -31,8 +31,6 @@ int xfs_link(struct xfs_inode *tdp, stru
> >  		struct xfs_name *target_name);
> >  int xfs_mkdir(struct xfs_inode *dp, struct xfs_name *dir_name,
> >  		mode_t mode, struct xfs_inode **ipp, struct cred *credp);
> > -int xfs_rmdir(struct xfs_inode *dp, struct xfs_name *name,
> > -		struct xfs_inode *cdp);
> >  int xfs_readdir(struct xfs_inode	*dp, void *dirent, size_t bufsize,
> >  		       xfs_off_t *offset, filldir_t filldir);
> >  int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,
> ---end quoted text---
---end quoted text---