Btrfs devel - Apr 2012 - [PATCH 1/4] vfs: introduce try_to_writeback_inodes_sb(_nr)

writeback_inodes_sb(_nr) grabs s_umount lock when it want to start writeback,
it may bring us deadlock problem when doing umount. So we introduce new
functions -- try_to_writeback_inodes_sb(_nr) -- which use down_read_trylock()
instead of down_read() to avoid that deadlock problem.

This idea came from Christoph Hellwig.
Some code is from the patch of Kamal Mostafa.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/fs-writeback.c         |   39 +++++++++++++++++++++++++++++++++++++++
 include/linux/writeback.h |    3 +++
 2 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 539f36c..b0c35c3 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1291,6 +1291,45 @@ int writeback_inodes_sb_nr_if_idle(struct super_block
*sb,
 EXPORT_SYMBOL(writeback_inodes_sb_nr_if_idle);
 
 /**
+ * try_to_writeback_inodes_sb_nr - try to start writeback if none underway
+ * @sb: the superblock
+ * @nr: the number of pages to write
+ * @reason: the reason of writeback
+ *
+ * Invoke writeback_inodes_sb_nr if no writeback is currently underway.
+ * Returns 1 if writeback was started, 0 if not.
+ */
+int try_to_writeback_inodes_sb_nr(struct super_block *sb,
+				  unsigned long nr,
+				  enum wb_reason reason)
+{
+	if (writeback_in_progress(sb->s_bdi))
+		return 1;
+
+	if (!down_read_trylock(&sb->s_umount))
+		return 0;
+
+	writeback_inodes_sb_nr(sb, nr, reason);
+	up_read(&sb->s_umount);
+	return 1;
+}
+EXPORT_SYMBOL(try_to_writeback_inodes_sb_nr);
+
+/**
+ * try_to_writeback_inodes_sb - try to start writeback if none underway
+ * @sb: the superblock
+ * @reason: reason why some writeback work was initiated
+ *
+ * Implement by try_to_writeback_inodes_sb_nr()
+ * Returns 1 if writeback was started, 0 if not.
+ */
+int try_to_writeback_inodes_sb(struct super_block *sb, enum wb_reason reason)
+{
+	return try_to_writeback_inodes_sb_nr(sb, get_nr_dirty_pages(), reason);
+}
+EXPORT_SYMBOL(try_to_writeback_inodes_sb);
+
+/**
  * sync_inodes_sb	-	sync sb inode pages
  * @sb: the superblock
  *
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index a2b84f5..d8e2a99 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -89,6 +89,9 @@ void writeback_inodes_sb_nr(struct super_block *, unsigned
long nr,
 int writeback_inodes_sb_if_idle(struct super_block *, enum wb_reason reason);
 int writeback_inodes_sb_nr_if_idle(struct super_block *, unsigned long nr,
 							enum wb_reason reason);
+int try_to_writeback_inodes_sb(struct super_block *, enum wb_reason reason);
+int try_to_writeback_inodes_sb_nr(struct super_block *, unsigned long nr,
+				  enum wb_reason reason);
 void sync_inodes_sb(struct super_block *);
 long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages,
 				enum wb_reason reason);
-- 
1.7.6.5
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Apr 26, 2012 at 10:57:43AM +0800, Miao Xie
wrote:
> writeback_inodes_sb(_nr) grabs s_umount lock when it want to start
writeback,
> it may bring us deadlock problem when doing umount. So we introduce new
> functions -- try_to_writeback_inodes_sb(_nr) -- which use
down_read_trylock()
> instead of down_read() to avoid that deadlock problem.
> 
> This idea came from Christoph Hellwig.
> Some code is from the patch of Kamal Mostafa.
This just re-implements writeback_inodes_[nr]_sb_if_idle() with a
trylock instead of a blocking lock.

Just replace the blocking lock in writeback_inodes_[nr]_sb_if_idle()
with a trylock and use that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
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

On Thu, Apr 26, 2012 at 11:11 AM, Dave Chinner <david@fromorbit.com>
wrote:
> > writeback_inodes_sb(_nr) grabs s_umount lock when it want to start
> > writeback,
> > it may bring us deadlock problem when doing umount. So we introduce
new
> > functions -- try_to_writeback_inodes_sb(_nr) -- which use
> > down_read_trylock()
> > instead of down_read() to avoid that deadlock problem.
> >
> > This idea came from Christoph Hellwig.
> > Some code is from the patch of Kamal Mostafa.
>
> This just re-implements writeback_inodes_[nr]_sb_if_idle() with a
> trylock instead of a blocking lock.
>
> Just replace the blocking lock in writeback_inodes_[nr]_sb_if_idle()
> with a trylock and use that.
The change of these two functions is relative to three modules,  so I think
the patch set now is easy to be reviewed by the developers of each module.

Thanks
Miao
--
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

On Thu, Apr 26, 2012 at 03:55:52PM +0800, Xie Miao
wrote:
> On Thu, Apr 26, 2012 at 11:11 AM, Dave Chinner <david@fromorbit.com>
wrote:
> > > writeback_inodes_sb(_nr) grabs s_umount lock when it want to
start
> > > writeback,
> > > it may bring us deadlock problem when doing umount. So we
introduce new
> > > functions -- try_to_writeback_inodes_sb(_nr) -- which use
> > > down_read_trylock()
> > > instead of down_read() to avoid that deadlock problem.
> > >
> > > This idea came from Christoph Hellwig.
> > > Some code is from the patch of Kamal Mostafa.
> >
> > This just re-implements writeback_inodes_[nr]_sb_if_idle() with a
> > trylock instead of a blocking lock.
> >
> > Just replace the blocking lock in writeback_inodes_[nr]_sb_if_idle()
> > with a trylock and use that.
> 
> The change of these two functions is relative to three modules,  so I think
> the patch set now is easy to be reviewed by the developers of each module.
> 
I agree with David, there''s no sense in making something completely
seperate,
this function was introduced soley to kick off background writeout if we could
with no garuntees, if the other users suddenly don''t like the behavior
they can
creating something different for themselves.  Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

于 2012年04月26日 23:12, Josef Bacik 写道:
> On Thu, Apr 26, 2012 at 03:55:52PM +0800, Xie Miao wrote:
>> On Thu, Apr 26, 2012 at 11:11 AM, Dave Chinner
<david@fromorbit.com> wrote:
>>>> writeback_inodes_sb(_nr) grabs s_umount lock when it want to
start
>>>> writeback,
>>>> it may bring us deadlock problem when doing umount. So we
introduce new
>>>> functions -- try_to_writeback_inodes_sb(_nr) -- which use
>>>> down_read_trylock()
>>>> instead of down_read() to avoid that deadlock problem.
>>>>
>>>> This idea came from Christoph Hellwig.
>>>> Some code is from the patch of Kamal Mostafa.
>>>
>>> This just re-implements writeback_inodes_[nr]_sb_if_idle() with a
>>> trylock instead of a blocking lock.
>>>
>>> Just replace the blocking lock in
writeback_inodes_[nr]_sb_if_idle()
>>> with a trylock and use that.
>>
>> The change of these two functions is relative to three modules,  so I
think
>> the patch set now is easy to be reviewed by the developers of each
module.
>>
> 
> I agree with David, there''s no sense in making something
completely seperate,
> this function was introduced soley to kick off background writeout if we
could
> with no garuntees, if the other users suddenly don''t like the
behavior they can
> creating something different for themselves.  Thanks,

OK, I''ll make them together.

Thanks
Miao
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html