xfs
[Top] [All Lists]

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

To: xfs@xxxxxxxxxxx
Subject: Re: [PATCH 1/2] merge xfs_rmdir into xfs_remove
From: Christoph Hellwig <hch@xxxxxx>
Date: Tue, 20 May 2008 08:36:31 +0200
In-reply-to: <20080502105757.GB17870@xxxxxx>
References: <20080502105757.GB17870@xxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.3.28i
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@xxxxxx>
> 
> 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---


<Prev in Thread] Current Thread [Next in Thread>