* [PATCH 43/58] switch ocfs2 to ->evict_inode()
@ 2010-06-08 22:22 Al Viro
2010-06-09 0:17 ` Joel Becker
2010-06-09 1:44 ` [PATCH 43/58][v2] " Al Viro
0 siblings, 2 replies; 6+ messages in thread
From: Al Viro @ 2010-06-08 22:22 UTC (permalink / raw)
To: linux-fsdevel
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/ocfs2/inode.c | 23 ++++++++---------------
fs/ocfs2/inode.h | 3 +--
fs/ocfs2/super.c | 3 +--
3 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index abb0a95..29343c9 100644
--- a/fs/ocfs2/inode.c
+++ b/fs/ocfs2/inode.c
@@ -969,14 +969,20 @@ static void ocfs2_cleanup_delete_inode(struct inode *inode,
truncate_inode_pages(&inode->i_data, 0);
}
-void ocfs2_delete_inode(struct inode *inode)
+void ocfs2_evict_inode(struct inode *inode)
{
int wipe, status;
sigset_t oldset;
struct buffer_head *di_bh = NULL;
+ struct ocfs2_inode_info *oi = OCFS2_I(inode);
mlog_entry("(inode->i_ino = %lu)\n", inode->i_ino);
+ if (inode->i_nlink) {
+ truncate_inode_pages(&inode->i_data, 0);
+ goto bail;
+ }
+
/* When we fail in read_inode() we mark inode as bad. The second test
* catches the case when inode allocation fails before allocating
* a block for inode. */
@@ -1075,19 +1081,7 @@ bail_unlock_nfs_sync:
bail_unblock:
ocfs2_unblock_signals(&oldset);
bail:
- clear_inode(inode);
- mlog_exit_void();
-}
-
-void ocfs2_clear_inode(struct inode *inode)
-{
- int status;
- struct ocfs2_inode_info *oi = OCFS2_I(inode);
-
- mlog_entry_void();
-
- if (!inode)
- goto bail;
+ end_writeback(inode);
mlog(0, "Clearing inode: %llu, nlink = %u\n",
(unsigned long long)OCFS2_I(inode)->ip_blkno, inode->i_nlink);
@@ -1180,7 +1174,6 @@ void ocfs2_clear_inode(struct inode *inode)
jbd2_journal_release_jbd_inode(OCFS2_SB(inode->i_sb)->journal->j_journal,
&oi->ip_jinode);
-bail:
mlog_exit_void();
}
diff --git a/fs/ocfs2/inode.h b/fs/ocfs2/inode.h
index 9f5f5fc..975eedd 100644
--- a/fs/ocfs2/inode.h
+++ b/fs/ocfs2/inode.h
@@ -123,8 +123,7 @@ static inline struct ocfs2_caching_info *INODE_CACHE(struct inode *inode)
return &OCFS2_I(inode)->ip_metadata_cache;
}
-void ocfs2_clear_inode(struct inode *inode);
-void ocfs2_delete_inode(struct inode *inode);
+void ocfs2_evict_inode(struct inode *inode);
void ocfs2_drop_inode(struct inode *inode);
/* Flags for ocfs2_iget() */
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 0eaa929..ae1a443 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -145,8 +145,7 @@ static const struct super_operations ocfs2_sops = {
.alloc_inode = ocfs2_alloc_inode,
.destroy_inode = ocfs2_destroy_inode,
.drop_inode = ocfs2_drop_inode,
- .clear_inode = ocfs2_clear_inode,
- .delete_inode = ocfs2_delete_inode,
+ .evict_inode = ocfs2_evict_inode,
.sync_fs = ocfs2_sync_fs,
.put_super = ocfs2_put_super,
.remount_fs = ocfs2_remount,
--
1.5.6.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 43/58] switch ocfs2 to ->evict_inode()
2010-06-08 22:22 [PATCH 43/58] switch ocfs2 to ->evict_inode() Al Viro
@ 2010-06-09 0:17 ` Joel Becker
2010-06-09 1:19 ` Al Viro
2010-06-09 1:44 ` [PATCH 43/58][v2] " Al Viro
1 sibling, 1 reply; 6+ messages in thread
From: Joel Becker @ 2010-06-09 0:17 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel, mfasheh
On Tue, Jun 08, 2010 at 11:22:03PM +0100, Al Viro wrote:
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
First and foremost, I like the ->evict_inode() idea. That's
really the state we're talking about; what to do when an inode is
leaving icache. The evict scheme is much less confusing about icache
lifetimes.
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index abb0a95..29343c9 100644
> --- a/fs/ocfs2/inode.c
> +++ b/fs/ocfs2/inode.c
> @@ -969,14 +969,20 @@ static void ocfs2_cleanup_delete_inode(struct inode *inode,
> truncate_inode_pages(&inode->i_data, 0);
> }
>
> -void ocfs2_delete_inode(struct inode *inode)
> +void ocfs2_evict_inode(struct inode *inode)
> {
> int wipe, status;
> sigset_t oldset;
> struct buffer_head *di_bh = NULL;
> + struct ocfs2_inode_info *oi = OCFS2_I(inode);
>
> mlog_entry("(inode->i_ino = %lu)\n", inode->i_ino);
>
> + if (inode->i_nlink) {
> + truncate_inode_pages(&inode->i_data, 0);
> + goto bail;
> + }
Gonna have to NAK this as is, unfortunately. This i_nlink check
is not safe. In the old code, we get into ->delete_inode() either when
the local i_nlink==0 (which your code handles just fine) or when
ocfs2_drop_inode() notices the MAYBE_ORPHANED flag and explicitly calls
generic_delete_inode(). In ocfs2_delete_inode(), we don't check our
i_nlink until ocfs2_query_wipe_inode(), which is safely under the
cluster lock.
So the old code, with its distinct delete_inode path, lets us
know deterministically when an inode must still be alive. Our old
ocfs2_clear_inode() can safely avoid taking any cluster locks. It only
has to free up and drop locks we're already holding.
Your new code is trivially functional by removing the i_nlink
check. The ocfs2_delete_inode() code is smart enough to realize the
inode is actually still alive after locking it down. That is, after
all, what we do in the MAYBE_ORPHANED case. However, that means every
single inode eviction has to take cluster locks we may not be holding.
That's not acceptable from a performance perspective.
I do think it works, though, if you check for MAYBE_ORPHANED.
if (inode->i_nlink && !(oi->ip_falgs & OCFS2_INODE_MAYBE_ORPHANED)) {
ocfs2_cleanup_delete_inode(inode, 0);
goto bail;
}
Yeah, that makes me happy.
> @@ -1075,19 +1081,7 @@ bail_unlock_nfs_sync:
> bail_unblock:
> ocfs2_unblock_signals(&oldset);
> bail:
> - clear_inode(inode);
> - mlog_exit_void();
> -}
> -
> -void ocfs2_clear_inode(struct inode *inode)
> -{
> - int status;
> - struct ocfs2_inode_info *oi = OCFS2_I(inode);
> -
> - mlog_entry_void();
> -
> - if (!inode)
> - goto bail;
> + end_writeback(inode);
The other thing that bothers me about your change is the
smushing of two long functions into a longer one. It just doesn't read
nicely to me.
What about making ocfs2_delete_inode() and ocfs2_clear_inode()
static and then adding:
void ocfs2_evict_inode(struct inode *inode)
{
if (!inode->i_nlink ||
(OCFS2_I(inode)->ip_flags & OCFS2_INODE_MAYBE_ORPHANED)) {
ocfs2_delete_inode(inode);
} else
truncate_inode_pages(&inode->i_data, 0);
ocfs2_clear_inode(inode);
}
end_writeback() gets added to ocfs2_clear_inode() as expected.
Joel
--
"There is shadow under this red rock.
(Come in under the shadow of this red rock)
And I will show you something different from either
Your shadow at morning striding behind you
Or your shadow at evening rising to meet you.
I will show you fear in a handful of dust."
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 43/58] switch ocfs2 to ->evict_inode()
2010-06-09 0:17 ` Joel Becker
@ 2010-06-09 1:19 ` Al Viro
2010-06-09 1:49 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2010-06-09 1:19 UTC (permalink / raw)
To: Al Viro, linux-fsdevel, mfasheh
On Tue, Jun 08, 2010 at 05:17:11PM -0700, Joel Becker wrote:
> I do think it works, though, if you check for MAYBE_ORPHANED.
>
> if (inode->i_nlink && !(oi->ip_falgs & OCFS2_INODE_MAYBE_ORPHANED)) {
> ocfs2_cleanup_delete_inode(inode, 0);
> goto bail;
> }
Damn, that probably means we have other problems like that. Other filesystems
that have extra "delete" cases (and do not play with i_nlink like gfs2 does)
and have non-trivial ->delete_inode().
Ho-hum... OK, not too many. btrfs seems to be the only one besides ocfs2.
Will fix...
I'll post replacement patches in a few.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 43/58] switch ocfs2 to ->evict_inode()
2010-06-09 1:19 ` Al Viro
@ 2010-06-09 1:49 ` Al Viro
0 siblings, 0 replies; 6+ messages in thread
From: Al Viro @ 2010-06-09 1:49 UTC (permalink / raw)
To: linux-fsdevel
On Wed, Jun 09, 2010 at 02:19:59AM +0100, Al Viro wrote:
> On Tue, Jun 08, 2010 at 05:17:11PM -0700, Joel Becker wrote:
> > I do think it works, though, if you check for MAYBE_ORPHANED.
> >
> > if (inode->i_nlink && !(oi->ip_falgs & OCFS2_INODE_MAYBE_ORPHANED)) {
> > ocfs2_cleanup_delete_inode(inode, 0);
> > goto bail;
> > }
>
> Damn, that probably means we have other problems like that. Other filesystems
> that have extra "delete" cases (and do not play with i_nlink like gfs2 does)
> and have non-trivial ->delete_inode().
>
> Ho-hum... OK, not too many. btrfs seems to be the only one besides ocfs2.
> Will fix...
>
> I'll post replacement patches in a few.
Posted and pushed to vfs-2.6.git...
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 43/58][v2] switch ocfs2 to ->evict_inode()
2010-06-08 22:22 [PATCH 43/58] switch ocfs2 to ->evict_inode() Al Viro
2010-06-09 0:17 ` Joel Becker
@ 2010-06-09 1:44 ` Al Viro
2010-06-09 1:53 ` Joel Becker
1 sibling, 1 reply; 6+ messages in thread
From: Al Viro @ 2010-06-09 1:44 UTC (permalink / raw)
To: linux-fsdevel
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index abb0a95..eb7fd07 100644
--- a/fs/ocfs2/inode.c
+++ b/fs/ocfs2/inode.c
@@ -969,7 +969,7 @@ static void ocfs2_cleanup_delete_inode(struct inode *inode,
truncate_inode_pages(&inode->i_data, 0);
}
-void ocfs2_delete_inode(struct inode *inode)
+static void ocfs2_delete_inode(struct inode *inode)
{
int wipe, status;
sigset_t oldset;
@@ -1075,20 +1075,17 @@ bail_unlock_nfs_sync:
bail_unblock:
ocfs2_unblock_signals(&oldset);
bail:
- clear_inode(inode);
mlog_exit_void();
}
-void ocfs2_clear_inode(struct inode *inode)
+static void ocfs2_clear_inode(struct inode *inode)
{
int status;
struct ocfs2_inode_info *oi = OCFS2_I(inode);
mlog_entry_void();
- if (!inode)
- goto bail;
-
+ end_writeback(inode);
mlog(0, "Clearing inode: %llu, nlink = %u\n",
(unsigned long long)OCFS2_I(inode)->ip_blkno, inode->i_nlink);
@@ -1180,10 +1177,20 @@ void ocfs2_clear_inode(struct inode *inode)
jbd2_journal_release_jbd_inode(OCFS2_SB(inode->i_sb)->journal->j_journal,
&oi->ip_jinode);
-bail:
mlog_exit_void();
}
+void ocfs2_evict_inode(struct inode *inode)
+{
+ if (!inode->i_nlink ||
+ (OCFS2_I(inode)->ip_flags & OCFS2_INODE_MAYBE_ORPHANED)) {
+ ocfs2_delete_inode(inode);
+ } else {
+ truncate_inode_pages(&inode->i_data, 0);
+ }
+ ocfs2_clear_inode(inode);
+}
+
/* Called under inode_lock, with no more references on the
* struct inode, so it's safe here to check the flags field
* and to manipulate i_nlink without any other locks. */
diff --git a/fs/ocfs2/inode.h b/fs/ocfs2/inode.h
index 9f5f5fc..975eedd 100644
--- a/fs/ocfs2/inode.h
+++ b/fs/ocfs2/inode.h
@@ -123,8 +123,7 @@ static inline struct ocfs2_caching_info *INODE_CACHE(struct inode *inode)
return &OCFS2_I(inode)->ip_metadata_cache;
}
-void ocfs2_clear_inode(struct inode *inode);
-void ocfs2_delete_inode(struct inode *inode);
+void ocfs2_evict_inode(struct inode *inode);
void ocfs2_drop_inode(struct inode *inode);
/* Flags for ocfs2_iget() */
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 0eaa929..ae1a443 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -145,8 +145,7 @@ static const struct super_operations ocfs2_sops = {
.alloc_inode = ocfs2_alloc_inode,
.destroy_inode = ocfs2_destroy_inode,
.drop_inode = ocfs2_drop_inode,
- .clear_inode = ocfs2_clear_inode,
- .delete_inode = ocfs2_delete_inode,
+ .evict_inode = ocfs2_evict_inode,
.sync_fs = ocfs2_sync_fs,
.put_super = ocfs2_put_super,
.remount_fs = ocfs2_remount,
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 43/58][v2] switch ocfs2 to ->evict_inode()
2010-06-09 1:44 ` [PATCH 43/58][v2] " Al Viro
@ 2010-06-09 1:53 ` Joel Becker
0 siblings, 0 replies; 6+ messages in thread
From: Joel Becker @ 2010-06-09 1:53 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel
On Wed, Jun 09, 2010 at 02:44:18AM +0100, Al Viro wrote:
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Acked-by: Joel Becker <joel.becker@oracle.com>
Let me know when you push this to your next branch. I want to
try and schedule some testing.
Joel
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index abb0a95..eb7fd07 100644
> --- a/fs/ocfs2/inode.c
> +++ b/fs/ocfs2/inode.c
> @@ -969,7 +969,7 @@ static void ocfs2_cleanup_delete_inode(struct inode *inode,
> truncate_inode_pages(&inode->i_data, 0);
> }
>
> -void ocfs2_delete_inode(struct inode *inode)
> +static void ocfs2_delete_inode(struct inode *inode)
> {
> int wipe, status;
> sigset_t oldset;
> @@ -1075,20 +1075,17 @@ bail_unlock_nfs_sync:
> bail_unblock:
> ocfs2_unblock_signals(&oldset);
> bail:
> - clear_inode(inode);
> mlog_exit_void();
> }
>
> -void ocfs2_clear_inode(struct inode *inode)
> +static void ocfs2_clear_inode(struct inode *inode)
> {
> int status;
> struct ocfs2_inode_info *oi = OCFS2_I(inode);
>
> mlog_entry_void();
>
> - if (!inode)
> - goto bail;
> -
> + end_writeback(inode);
> mlog(0, "Clearing inode: %llu, nlink = %u\n",
> (unsigned long long)OCFS2_I(inode)->ip_blkno, inode->i_nlink);
>
> @@ -1180,10 +1177,20 @@ void ocfs2_clear_inode(struct inode *inode)
> jbd2_journal_release_jbd_inode(OCFS2_SB(inode->i_sb)->journal->j_journal,
> &oi->ip_jinode);
>
> -bail:
> mlog_exit_void();
> }
>
> +void ocfs2_evict_inode(struct inode *inode)
> +{
> + if (!inode->i_nlink ||
> + (OCFS2_I(inode)->ip_flags & OCFS2_INODE_MAYBE_ORPHANED)) {
> + ocfs2_delete_inode(inode);
> + } else {
> + truncate_inode_pages(&inode->i_data, 0);
> + }
> + ocfs2_clear_inode(inode);
> +}
> +
> /* Called under inode_lock, with no more references on the
> * struct inode, so it's safe here to check the flags field
> * and to manipulate i_nlink without any other locks. */
> diff --git a/fs/ocfs2/inode.h b/fs/ocfs2/inode.h
> index 9f5f5fc..975eedd 100644
> --- a/fs/ocfs2/inode.h
> +++ b/fs/ocfs2/inode.h
> @@ -123,8 +123,7 @@ static inline struct ocfs2_caching_info *INODE_CACHE(struct inode *inode)
> return &OCFS2_I(inode)->ip_metadata_cache;
> }
>
> -void ocfs2_clear_inode(struct inode *inode);
> -void ocfs2_delete_inode(struct inode *inode);
> +void ocfs2_evict_inode(struct inode *inode);
> void ocfs2_drop_inode(struct inode *inode);
>
> /* Flags for ocfs2_iget() */
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index 0eaa929..ae1a443 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -145,8 +145,7 @@ static const struct super_operations ocfs2_sops = {
> .alloc_inode = ocfs2_alloc_inode,
> .destroy_inode = ocfs2_destroy_inode,
> .drop_inode = ocfs2_drop_inode,
> - .clear_inode = ocfs2_clear_inode,
> - .delete_inode = ocfs2_delete_inode,
> + .evict_inode = ocfs2_evict_inode,
> .sync_fs = ocfs2_sync_fs,
> .put_super = ocfs2_put_super,
> .remount_fs = ocfs2_remount,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Life's Little Instruction Book #337
"Reread your favorite book."
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-06-09 1:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-08 22:22 [PATCH 43/58] switch ocfs2 to ->evict_inode() Al Viro
2010-06-09 0:17 ` Joel Becker
2010-06-09 1:19 ` Al Viro
2010-06-09 1:49 ` Al Viro
2010-06-09 1:44 ` [PATCH 43/58][v2] " Al Viro
2010-06-09 1:53 ` Joel Becker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).