[PATCH 7/58] new helper: end_writeback()

[PATCH 7/58] new helper: end_writeback()

[Al Viro]

Essentially, the minimal variant of ->evict_inode().  It's
a trimmed-down clear_inode(), sans any fs callbacks.  Once
it returns we know that no async writeback will be happening;
every ->evict_inode() instance should do that once and do that
before doing anything ->write_inode() could interfere with
(e.g. freeing the on-disk inode).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/hugetlbfs/inode.c |    2 +-
 fs/inode.c           |   12 ++++++++++++
 include/linux/fs.h   |    1 +
 3 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index bf1a2f4..6e5bd42 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -374,7 +374,7 @@ static void truncate_hugepages(struct inode *inode, loff_t lstart)
 static void hugetlbfs_evict_inode(struct inode *inode)
 {
 	truncate_hugepages(inode, 0);
-	clear_inode(inode);
+	end_writeback(inode);
 }
 
 static inline void
diff --git a/fs/inode.c b/fs/inode.c
index 139d475..3f45360 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -294,6 +294,18 @@ void __iget(struct inode *inode)
 	inodes_stat.nr_unused--;
 }
 
+void end_writeback(struct inode *inode)
+{
+	might_sleep();
+	BUG_ON(inode->i_data.nrpages);
+	BUG_ON(!list_empty(&inode->i_data.private_list));
+	BUG_ON(!(inode->i_state & I_FREEING));
+	BUG_ON(inode->i_state & I_CLEAR);
+	inode_sync_wait(inode);
+	inode->i_state = I_FREEING | I_CLEAR;
+}
+EXPORT_SYMBOL(end_writeback);
+
 /**
  * clear_inode - clear an inode
  * @inode: inode to clear
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 788b003..cea7283 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2170,6 +2170,7 @@ extern void unlock_new_inode(struct inode *);
 extern void __iget(struct inode * inode);
 extern void iget_failed(struct inode *);
 extern void clear_inode(struct inode *);
+extern void end_writeback(struct inode *);
 extern void destroy_inode(struct inode *);
 extern void __destroy_inode(struct inode *);
 extern struct inode *new_inode(struct super_block *);
-- 
1.5.6.5

Re: [PATCH 7/58] new helper: end_writeback()

[Christoph Hellwig]

On Tue, Jun 08, 2010 at 11:16:02PM +0100, Al Viro wrote:
> 
> Essentially, the minimal variant of ->evict_inode().  It's
> a trimmed-down clear_inode(), sans any fs callbacks.  Once
> it returns we know that no async writeback will be happening;
> every ->evict_inode() instance should do that once and do that
> before doing anything ->write_inode() could interfere with
> (e.g. freeing the on-disk inode).

Naming seems a bit unfortunate - this really sounds like something
in page writeback.  In fact I'd almost bet we had a function with
that name there in the past.

Care to slap an inode_ prefix on it?

Re: [PATCH 7/58] new helper: end_writeback()

[Al Viro]

On Wed, Jun 09, 2010 at 09:02:16AM -0400, Christoph Hellwig wrote:
> On Tue, Jun 08, 2010 at 11:16:02PM +0100, Al Viro wrote:
> > 
> > Essentially, the minimal variant of ->evict_inode().  It's
> > a trimmed-down clear_inode(), sans any fs callbacks.  Once
> > it returns we know that no async writeback will be happening;
> > every ->evict_inode() instance should do that once and do that
> > before doing anything ->write_inode() could interfere with
> > (e.g. freeing the on-disk inode).
> 
> Naming seems a bit unfortunate - this really sounds like something
> in page writeback.  In fact I'd almost bet we had a function with
> that name there in the past.
> 
> Care to slap an inode_ prefix on it?

Ehh...  I'd been tempted to call it end_async or something like that.
I'm not particulary fond of the name; any better suggestions are welcome.

The point of what's being done in that function is that it acts as a barrier;
"wait for async activity to run down; new one won't be started since it's
already marked I_FREEING".

Re: [PATCH 7/58] new helper: end_writeback()

[Boaz Harrosh]

On 06/09/2010 04:20 PM, Al Viro wrote:
> On Wed, Jun 09, 2010 at 09:02:16AM -0400, Christoph Hellwig wrote:
>> On Tue, Jun 08, 2010 at 11:16:02PM +0100, Al Viro wrote:
>>>
>>> Essentially, the minimal variant of ->evict_inode().  It's
>>> a trimmed-down clear_inode(), sans any fs callbacks.  Once
>>> it returns we know that no async writeback will be happening;
>>> every ->evict_inode() instance should do that once and do that
>>> before doing anything ->write_inode() could interfere with
>>> (e.g. freeing the on-disk inode).
>>
>> Naming seems a bit unfortunate - this really sounds like something
>> in page writeback.  In fact I'd almost bet we had a function with
>> that name there in the past.
>>
>> Care to slap an inode_ prefix on it?
> 
> Ehh...  I'd been tempted to call it end_async or something like that.
> I'm not particulary fond of the name; any better suggestions are welcome.
> 
> The point of what's being done in that function is that it acts as a barrier;
> "wait for async activity to run down; new one won't be started since it's
> already marked I_FREEING".

It was odd for me to. I had to go look at code to understand what it's for.
I would prefer the word "wait" in the name or barrier. eg. wait_writeback.
end_writeback sounds like something you should call from ->write_end

Just my $0.017
Boaz