[PATCH 0/2] ubifs: respect dirty_writeback_interval

[PATCH 0/2] ubifs: respect dirty_writeback_interval

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Hi,

This patchset allows some ubifs adjustments that we realized we needed
in LEDE project.

LEDE can be unstable on power cuts when installed on NAND devices (with
ubifs). This is caused by the default high value of wbuf timeout used by
ubifs. Any write that isn't followed by fsync (this may happen with
buggy user space app or just shell script) may be lost if power cut
happens in less than 5 seconds.

One idea for fixing this (without modifying kernel at all) is to mount
ubifs with -o sync. This could affect NAND performance however, so I'm
looking for a better solution.

During IRC discussion MTD guys suggested lowering wbuf timeout and I
decided to give it a try. The simplest way to do that seems to be making
ubifs respect dirty_writeback_interval. This parameter can be easily set
with sysctl and is already used in some older file systems.

What do you think about this? I gave it a try with a simple
uci commit foo; sleep 1s; POWER_CUT
and it works as expected.

Rafał Miłecki (2):
  ubifs: drop softlimit and delta fields from struct ubifs_wbuf
  ubifs: use dirty_writeback_interval value for wbuf timer

 fs/ubifs/io.c    | 15 +++++++--------
 fs/ubifs/ubifs.h |  9 ---------
 2 files changed, 7 insertions(+), 17 deletions(-)

-- 
2.9.3

Re: [PATCH 0/2] ubifs: respect dirty_writeback_interval

[Richard Weinberger]

Rafał,

On 14.09.2016 12:21, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Hi,
> 
> This patchset allows some ubifs adjustments that we realized we needed
> in LEDE project.
> 
> LEDE can be unstable on power cuts when installed on NAND devices (with
> ubifs). This is caused by the default high value of wbuf timeout used by
> ubifs. Any write that isn't followed by fsync (this may happen with
> buggy user space app or just shell script) may be lost if power cut
> happens in less than 5 seconds.
> 
> One idea for fixing this (without modifying kernel at all) is to mount
> ubifs with -o sync. This could affect NAND performance however, so I'm
> looking for a better solution.
> 
> During IRC discussion MTD guys suggested lowering wbuf timeout and I
> decided to give it a try. The simplest way to do that seems to be making
> ubifs respect dirty_writeback_interval. This parameter can be easily set
> with sysctl and is already used in some older file systems.
> 
> What do you think about this? I gave it a try with a simple
> uci commit foo; sleep 1s; POWER_CUT
> and it works as expected.

I think it is correct to tie the wbuf timeout to dirty_writeback_interval.
Especially since jffs2 does the same.
Please address the comments Boris raised. Then we can get this merged.

On the other hand, IMHO you will abuse this feature.
If your uci tool does not correctly fsync()/fdatasync() it is just broken and
needs fixing. Full stop.
Lowering the wbut timeout reduces the chance to lose unsynced data but it
won't fix the root cause. Users that face random power cuts will still have a
chance to lose data. Boris also pointed that out on IRC.

Thanks,
//richard

Re: [PATCH 0/2] ubifs: respect dirty_writeback_interval

[Rafał Miłecki]

On 16 September 2016 at 15:53, Richard Weinberger <richard@nod.at> wrote:
> On 14.09.2016 12:21, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Hi,
>>
>> This patchset allows some ubifs adjustments that we realized we needed
>> in LEDE project.
>>
>> LEDE can be unstable on power cuts when installed on NAND devices (with
>> ubifs). This is caused by the default high value of wbuf timeout used by
>> ubifs. Any write that isn't followed by fsync (this may happen with
>> buggy user space app or just shell script) may be lost if power cut
>> happens in less than 5 seconds.
>>
>> One idea for fixing this (without modifying kernel at all) is to mount
>> ubifs with -o sync. This could affect NAND performance however, so I'm
>> looking for a better solution.
>>
>> During IRC discussion MTD guys suggested lowering wbuf timeout and I
>> decided to give it a try. The simplest way to do that seems to be making
>> ubifs respect dirty_writeback_interval. This parameter can be easily set
>> with sysctl and is already used in some older file systems.
>>
>> What do you think about this? I gave it a try with a simple
>> uci commit foo; sleep 1s; POWER_CUT
>> and it works as expected.
>
> I think it is correct to tie the wbuf timeout to dirty_writeback_interval.
> Especially since jffs2 does the same.
> Please address the comments Boris raised. Then we can get this merged.
>
> On the other hand, IMHO you will abuse this feature.
> If your uci tool does not correctly fsync()/fdatasync() it is just broken and
> needs fixing. Full stop.
> Lowering the wbut timeout reduces the chance to lose unsynced data but it
> won't fix the root cause. Users that face random power cuts will still have a
> chance to lose data. Boris also pointed that out on IRC.

It was Felix who said we don't need to fix uci after all, but I'll try
to smuggle patch for it anyway. I hope Felix will consider it
noninvasive and maybe accept it.

For me it's more about other (broken) user space apps that users may
use but obviously we can't test them all.

I'll obviously update patch after Boris's comments, I was just waiting
to see if there will be any others. Thanks.

-- 
Rafał

[PATCH V2 1/2] ubifs: drop softlimit and delta fields from struct ubifs_wbuf

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Values of these fields are set during init and never modified. They are
used (read) in a single function only. There isn't really any reason to
keep them in a struct. It only makes struct just a bit bigger without
any visible gain.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 fs/ubifs/io.c    | 18 ++++++++++--------
 fs/ubifs/ubifs.h |  5 -----
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
index 97be412..4d6ce4a 100644
--- a/fs/ubifs/io.c
+++ b/fs/ubifs/io.c
@@ -452,16 +452,22 @@ static enum hrtimer_restart wbuf_timer_callback_nolock(struct hrtimer *timer)
  */
 static void new_wbuf_timer_nolock(struct ubifs_wbuf *wbuf)
 {
+	ktime_t softlimit = ktime_set(WBUF_TIMEOUT_SOFTLIMIT, 0);
+	unsigned long long delta;
+
+	delta = WBUF_TIMEOUT_HARDLIMIT - WBUF_TIMEOUT_SOFTLIMIT;
+	delta *= 1000000000ULL;
+
 	ubifs_assert(!hrtimer_active(&wbuf->timer));
+	ubifs_assert(delta <= ULONG_MAX);
 
 	if (wbuf->no_timer)
 		return;
 	dbg_io("set timer for jhead %s, %llu-%llu millisecs",
 	       dbg_jhead(wbuf->jhead),
-	       div_u64(ktime_to_ns(wbuf->softlimit), USEC_PER_SEC),
-	       div_u64(ktime_to_ns(wbuf->softlimit) + wbuf->delta,
-		       USEC_PER_SEC));
-	hrtimer_start_range_ns(&wbuf->timer, wbuf->softlimit, wbuf->delta,
+	       div_u64(ktime_to_ns(softlimit), USEC_PER_SEC),
+	       div_u64(ktime_to_ns(softlimit) + delta, USEC_PER_SEC));
+	hrtimer_start_range_ns(&wbuf->timer, softlimit, delta,
 			       HRTIMER_MODE_REL);
 }
 
@@ -1059,10 +1065,6 @@ int ubifs_wbuf_init(struct ubifs_info *c, struct ubifs_wbuf *wbuf)
 
 	hrtimer_init(&wbuf->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	wbuf->timer.function = wbuf_timer_callback_nolock;
-	wbuf->softlimit = ktime_set(WBUF_TIMEOUT_SOFTLIMIT, 0);
-	wbuf->delta = WBUF_TIMEOUT_HARDLIMIT - WBUF_TIMEOUT_SOFTLIMIT;
-	wbuf->delta *= 1000000000ULL;
-	ubifs_assert(wbuf->delta <= ULONG_MAX);
 	return 0;
 }
 
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 4617d45..11bc8fa 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -644,9 +644,6 @@ typedef int (*ubifs_lpt_scan_callback)(struct ubifs_info *c,
  * @io_mutex: serializes write-buffer I/O
  * @lock: serializes @buf, @lnum, @offs, @avail, @used, @next_ino and @inodes
  *        fields
- * @softlimit: soft write-buffer timeout interval
- * @delta: hard and soft timeouts delta (the timer expire interval is @softlimit
- *         and @softlimit + @delta)
  * @timer: write-buffer timer
  * @no_timer: non-zero if this write-buffer does not have a timer
  * @need_sync: non-zero if the timer expired and the wbuf needs sync'ing
@@ -675,8 +672,6 @@ struct ubifs_wbuf {
 	int (*sync_callback)(struct ubifs_info *c, int lnum, int free, int pad);
 	struct mutex io_mutex;
 	spinlock_t lock;
-	ktime_t softlimit;
-	unsigned long long delta;
 	struct hrtimer timer;
 	unsigned int no_timer:1;
 	unsigned int need_sync:1;
-- 
2.9.3

[PATCH V2 2/2] ubifs: use dirty_writeback_interval value for wbuf timer

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Right now wbuf timer has hardcoded timeouts and there is no place for
manual adjustments. Some projects / cases many need that though. Few
file systems allow doing that by respecting dirty_writeback_interval
that can be set using sysctl (dirty_writeback_centisecs).

Lowering dirty_writeback_interval could be some way of dealing with user
space apps lacking proper fsyncs. This is definitely *not* a perfect
solution but we don't have ideal (user space) world. There were already
advanced discussions on this matter, mostly when ext4 was introduced and
it wasn't behaving as ext3. Anyway, the final decision was to add some
hacks to the ext4, as trying to fix whole user space or adding new API
was pointless.

We can't (and shouldn't?) just follow ext4. We can't e.g. sync on close
as this would cause too many commits and flash wearing. On the other
hand we still should allow some trade-off between -o sync and default
wbuf timeout. Respecting dirty_writeback_interval should allow some sane
cutomizations if used warily.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Fix delta calculation and make it more clear. Thanks Boris.
---
 fs/ubifs/io.c    | 8 ++++----
 fs/ubifs/ubifs.h | 4 ----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
index 4d6ce4a..3be2890 100644
--- a/fs/ubifs/io.c
+++ b/fs/ubifs/io.c
@@ -452,11 +452,11 @@ static enum hrtimer_restart wbuf_timer_callback_nolock(struct hrtimer *timer)
  */
 static void new_wbuf_timer_nolock(struct ubifs_wbuf *wbuf)
 {
-	ktime_t softlimit = ktime_set(WBUF_TIMEOUT_SOFTLIMIT, 0);
-	unsigned long long delta;
+	ktime_t softlimit = ms_to_ktime(dirty_writeback_interval * 10);
+	unsigned long long delta = dirty_writeback_interval;
 
-	delta = WBUF_TIMEOUT_HARDLIMIT - WBUF_TIMEOUT_SOFTLIMIT;
-	delta *= 1000000000ULL;
+	/* centi to milli, milli to nano, then 10% */
+	delta *= 10ULL * NSEC_PER_MSEC / 10ULL;
 
 	ubifs_assert(!hrtimer_active(&wbuf->timer));
 	ubifs_assert(delta <= ULONG_MAX);
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 11bc8fa..26e6340 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -83,10 +83,6 @@
  */
 #define BGT_NAME_PATTERN "ubifs_bgt%d_%d"
 
-/* Write-buffer synchronization timeout interval in seconds */
-#define WBUF_TIMEOUT_SOFTLIMIT 3
-#define WBUF_TIMEOUT_HARDLIMIT 5
-
 /* Maximum possible inode number (only 32-bit inodes are supported now) */
 #define MAX_INUM 0xFFFFFFFF
 
-- 
2.9.3

Re: [PATCH V2 2/2] ubifs: use dirty_writeback_interval value for wbuf timer

[Boris Brezillon]

On Tue, 20 Sep 2016 10:36:15 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Right now wbuf timer has hardcoded timeouts and there is no place for
> manual adjustments. Some projects / cases many need that though. Few
> file systems allow doing that by respecting dirty_writeback_interval
> that can be set using sysctl (dirty_writeback_centisecs).
> 
> Lowering dirty_writeback_interval could be some way of dealing with user
> space apps lacking proper fsyncs. This is definitely *not* a perfect
> solution but we don't have ideal (user space) world. There were already
> advanced discussions on this matter, mostly when ext4 was introduced and
> it wasn't behaving as ext3. Anyway, the final decision was to add some
> hacks to the ext4, as trying to fix whole user space or adding new API
> was pointless.
> 
> We can't (and shouldn't?) just follow ext4. We can't e.g. sync on close
> as this would cause too many commits and flash wearing. On the other
> hand we still should allow some trade-off between -o sync and default
> wbuf timeout. Respecting dirty_writeback_interval should allow some sane
> cutomizations if used warily.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
> V2: Fix delta calculation and make it more clear. Thanks Boris.
> ---
>  fs/ubifs/io.c    | 8 ++++----
>  fs/ubifs/ubifs.h | 4 ----
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
> index 4d6ce4a..3be2890 100644
> --- a/fs/ubifs/io.c
> +++ b/fs/ubifs/io.c
> @@ -452,11 +452,11 @@ static enum hrtimer_restart wbuf_timer_callback_nolock(struct hrtimer *timer)
>   */
>  static void new_wbuf_timer_nolock(struct ubifs_wbuf *wbuf)
>  {
> -	ktime_t softlimit = ktime_set(WBUF_TIMEOUT_SOFTLIMIT, 0);
> -	unsigned long long delta;
> +	ktime_t softlimit = ms_to_ktime(dirty_writeback_interval * 10);
> +	unsigned long long delta = dirty_writeback_interval;
>  
> -	delta = WBUF_TIMEOUT_HARDLIMIT - WBUF_TIMEOUT_SOFTLIMIT;
> -	delta *= 1000000000ULL;
> +	/* centi to milli, milli to nano, then 10% */
> +	delta *= 10ULL * NSEC_PER_MSEC / 10ULL;
>  
>  	ubifs_assert(!hrtimer_active(&wbuf->timer));
>  	ubifs_assert(delta <= ULONG_MAX);
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 11bc8fa..26e6340 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -83,10 +83,6 @@
>   */
>  #define BGT_NAME_PATTERN "ubifs_bgt%d_%d"
>  
> -/* Write-buffer synchronization timeout interval in seconds */
> -#define WBUF_TIMEOUT_SOFTLIMIT 3
> -#define WBUF_TIMEOUT_HARDLIMIT 5
> -
>  /* Maximum possible inode number (only 32-bit inodes are supported now) */
>  #define MAX_INUM 0xFFFFFFFF
>  

Re: [PATCH V2 1/2] ubifs: drop softlimit and delta fields from struct ubifs_wbuf

[Rafał Miłecki]

On 20 September 2016 at 10:36, Rafał Miłecki <zajec5@gmail.com> wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Values of these fields are set during init and never modified. They are
> used (read) in a single function only. There isn't really any reason to
> keep them in a struct. It only makes struct just a bit bigger without
> any visible gain.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Hi Richard, could you pick this small patchset, please?