2009-01-05 22:10:01

by Randy Dunlap

[permalink] [raw]
Subject: Re: mmotm 2009-01-05-12-50 uploaded (ubifs)

[email protected] wrote:
> The mm-of-the-moment snapshot 2009-01-05-12-50 has been uploaded to
>
> http://userweb.kernel.org/~akpm/mmotm/
>
> and will soon be available at
>
> git://git.zen-sources.org/zen/mmotm.git


mmotm-2009-0105-1250/fs/ubifs/super.c:435: error: 'WB_SYNC_HOLD' undeclared (first use in this function)


~Randy


2009-01-05 22:25:13

by Andrew Morton

[permalink] [raw]
Subject: Re: mmotm 2009-01-05-12-50 uploaded (ubifs)

On Mon, 05 Jan 2009 14:09:25 -0800
Randy Dunlap <[email protected]> wrote:

> [email protected] wrote:
> > The mm-of-the-moment snapshot 2009-01-05-12-50 has been uploaded to
> >
> > http://userweb.kernel.org/~akpm/mmotm/
> >
> > and will soon be available at
> >
> > git://git.zen-sources.org/zen/mmotm.git
>
>
> mmotm-2009-0105-1250/fs/ubifs/super.c:435: error: 'WB_SYNC_HOLD' undeclared (first use in this function)
>

It's amazing how much shiny new code turns up during the merge window
and screws things up.


commit 304d427cd99eb645b44b08d77e70ce308e6bcd8c
Author: Artem Bityutskiy <[email protected]>
AuthorDate: Sun Dec 28 08:04:17 2008 +0200
Commit: Artem Bityutskiy <[email protected]>
CommitDate: Wed Dec 31 14:13:24 2008 +0200

UBIFS: fix file-system synchronization

Argh. The ->sync_fs call is called _before_ all inodes are flushed.
This means we first sync write buffers and commit, then all
inodes are synced, and we end up with unflushed write buffers!

Fix this by forcing synching all indoes from 'ubifs_sync_fs()'.

Signed-off-by: Artem Bityutskiy <[email protected]>

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 1309783..4713017 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -35,6 +35,7 @@
#include <linux/seq_file.h>
#include <linux/mount.h>
#include <linux/math64.h>
+#include <linux/writeback.h>
#include "ubifs.h"

/*
@@ -431,6 +432,23 @@ static int ubifs_sync_fs(struct super_block *sb, int wait)
struct ubifs_info *c = sb->s_fs_info;
int i, ret = 0, err;
long long bud_bytes;
+ struct writeback_control wbc = {
+ .sync_mode = wait ? WB_SYNC_ALL : WB_SYNC_HOLD,
+ .range_start = 0,
+ .range_end = LLONG_MAX,
+ .nr_to_write = LONG_MAX,
+ };
+
+ /*
+ * VFS calls '->sync_fs()' before synchronizing all dirty inodes and
+ * pages, so synchronize them first, then commit the journal. Strictly
+ * speaking, it is not necessary to commit the journal here,
+ * synchronizing write-buffers would be enough. But committing makes
+ * UBIFS free space predictions much more accurate, so we want to let
+ * the user be able to get more accurate results of 'statfs()' after
+ * they synchronize the file system.
+ */
+ generic_sync_sb_inodes(sb, &wbc);

if (c->jheads) {
for (i = 0; i < c->jhead_cnt; i++) {


WB_SYNC_HOLD got removed by
http://userweb.kernel.org/~akpm/mmotm/broken-out/fs-remove-wb_sync_hold.patch

I think I'll just switch that to WB_SYNC_NONE. The `wait==0' mode is
just an advisory thing to help the fs shove lots of data into the
queues. If some gets missed then it'll be picked up on the second
->sync_fs call, with wait==1.


2009-01-06 12:12:15

by Artem Bityutskiy

[permalink] [raw]
Subject: Re: mmotm 2009-01-05-12-50 uploaded (ubifs)

On Mon, 2009-01-05 at 14:24 -0800, Andrew Morton wrote:
> WB_SYNC_HOLD got removed by
> http://userweb.kernel.org/~akpm/mmotm/broken-out/fs-remove-wb_sync_hold.patch
>
> I think I'll just switch that to WB_SYNC_NONE. The `wait==0' mode is
> just an advisory thing to help the fs shove lots of data into the
> queues. If some gets missed then it'll be picked up on the second
> ->sync_fs call, with wait==1.

Sorry for the problems caused by this patch. Here is a fix for your
convenience. Will you send it to Linus or I should do this?

From: Artem Bityutskiy <[email protected]>
Subject: [PATCH] UBIFS: do not use WB_SYNC_HOLD

WB_SYNC_HOLD is going to be zapped so we should not use it. Use
%WB_SYNC_NONE instead. Here is what akpm said:

"I think I'll just switch that to WB_SYNC_NONE. The `wait==0' mode is
just an advisory thing to help the fs shove lots of data into the
queues. If some gets missed then it'll be picked up on the second
->sync_fs call, with wait==1."

Thanks to Randy Dunlap <[email protected]> for catching this.

Signed-off-by: Artem Bityutskiy <[email protected]>
---
fs/ubifs/super.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 0d7564b..89556ee 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -432,12 +432,19 @@ static int ubifs_sync_fs(struct super_block *sb, int wait)
int i, err;
struct ubifs_info *c = sb->s_fs_info;
struct writeback_control wbc = {
- .sync_mode = wait ? WB_SYNC_ALL : WB_SYNC_HOLD,
+ .sync_mode = wait ? WB_SYNC_ALL : WB_SYNC_NONE,
.range_start = 0,
.range_end = LLONG_MAX,
.nr_to_write = LONG_MAX,
};

+ /*
+ * Note by akpm about WB_SYNC_NONE used above: zero @wait is just an
+ * advisory thing to help the file system shove lots of data into the
+ * queues. If some gets missed then it'll be picked up on the second
+ * '->sync_fs()' call, with non-zero @wait.
+ */
+
if (sb->s_flags & MS_RDONLY)
return 0;

--
1.6.0.6

--
Best regards,
Artem Bityutskiy (Битюцкий Артём)

2009-01-06 17:14:31

by Andrew Morton

[permalink] [raw]
Subject: Re: mmotm 2009-01-05-12-50 uploaded (ubifs)

On Tue, 06 Jan 2009 14:11:44 +0200 Artem Bityutskiy <[email protected]> wrote:

> On Mon, 2009-01-05 at 14:24 -0800, Andrew Morton wrote:
> > WB_SYNC_HOLD got removed by
> > http://userweb.kernel.org/~akpm/mmotm/broken-out/fs-remove-wb_sync_hold.patch
> >
> > I think I'll just switch that to WB_SYNC_NONE. The `wait==0' mode is
> > just an advisory thing to help the fs shove lots of data into the
> > queues. If some gets missed then it'll be picked up on the second
> > ->sync_fs call, with wait==1.
>
> Sorry for the problems caused by this patch. Here is a fix for your
> convenience. Will you send it to Linus or I should do this?
>
> From: Artem Bityutskiy <[email protected]>
> Subject: [PATCH] UBIFS: do not use WB_SYNC_HOLD

I can take care of that, thanks.