xfs
[Top] [All Lists]

Re: [PATCH 7/7] XFS: don't use vnodes where unnecessary

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 7/7] XFS: don't use vnodes where unnecessary
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 14 Aug 2008 16:10:22 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1218698083-11226-8-git-send-email-david@xxxxxxxxxxxxx>
References: <1218698083-11226-1-git-send-email-david@xxxxxxxxxxxxx> <1218698083-11226-8-git-send-email-david@xxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
>       do {
> +             struct inode    *inode;
> +             boolean_t       vnode_refed;
> +             xfs_inode_t     *ip = NULL;

This should not be inode_refed, no? :)

> -             vp = VFS_I(ip);
> -             if (!vp) {
> +             if (xfs_iflags_test(ip, (XFS_IRECLAIM|XFS_IRECLAIMABLE))) {

These changes really need to be folded into the previous patch for
bisectability reasons..

> +             inode = VFS_I(ip);
> +             if (VN_BAD(inode)) {

Just use is_bad_inode directly.  (Also in a few other places)

>               } else {
> -                     /* safe to unlock here as we have a reference */
> +                     if (!xfs_ilock_nowait(ip, lock_flags)) {
> +                             /* leave it to reclaim */
> +                             read_unlock(&pag->pag_ici_lock);
> +                             continue;
> +                     }
>                       read_unlock(&pag->pag_ici_lock);
>               }

Maybe not for this patch, but in general, why do we even bother about
the inodes we can't get a reference to, shouldn't we just always leave
this to reclaim?

> -             if ((flags & SYNC_DELWRI) && VN_DIRTY(vp)) {
> +             if ((flags & SYNC_DELWRI) && VN_DIRTY(VFS_I(ip))) {

Why not use the "inode" variable we have in scope anyway?

> +     if (!(inode->i_state & I_CLEAR))
> +             return atomic_read(&inode->i_count);

Well, it's just zero when the inode is out of the vfs, so we could
probably simply do it unconditional.

> +             vp = vn_grab(VFS_I(ip));
> +             if (vp) {

Please switch this one to igrab, and the inode name, too.  As this
is the last caller of vn_grab we can kill it now.


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