diff mbox series

[6/8,v3] ubifs: Use dirty_writeback_interval value for wbuf timer

Message ID 20190214132403.10687-7-linus.walleij@linaro.org
State New
Headers show
Series Stable material from OpenWrt for v4.9.y | expand

Commit Message

Linus Walleij Feb. 14, 2019, 1:24 p.m. UTC 
From: Rafał Miłecki <rafal@milecki.pl>


commit 1b7fc2c0069f3864a3dda15430b7aded31c0bfcc upstream.

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>

Signed-off-by: Richard Weinberger <richard@nod.at>

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
- This was applied upstream in v4.10
- Should be applied to stable v4.9.y
---
 fs/ubifs/io.c    | 8 ++++----
 fs/ubifs/ubifs.h | 4 ----
 2 files changed, 4 insertions(+), 8 deletions(-)

-- 
2.20.1

Comments

Sasha Levin Feb. 17, 2019, 6:18 p.m. UTC | #1 
On Thu, Feb 14, 2019 at 02:24:01PM +0100, Linus Walleij wrote:
>From: Rafał Miłecki <rafal@milecki.pl>

>

>commit 1b7fc2c0069f3864a3dda15430b7aded31c0bfcc upstream.

>

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

>Signed-off-by: Richard Weinberger <richard@nod.at>

>Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


This one looks like a new feature that will also require changes to
userspace. Is there actual breakage this fixes?

--
Thanks,
Sasha
Richard Weinberger Feb. 17, 2019, 6:49 p.m. UTC | #2 
Am Sonntag, 17. Februar 2019, 19:18:02 CET schrieb Sasha Levin:
> On Thu, Feb 14, 2019 at 02:24:01PM +0100, Linus Walleij wrote:

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

> >

> >commit 1b7fc2c0069f3864a3dda15430b7aded31c0bfcc upstream.

> >

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

> >Signed-off-by: Richard Weinberger <richard@nod.at>

> >Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> 

> This one looks like a new feature that will also require changes to

> userspace. Is there actual breakage this fixes?


IIRC that's why I never tagged it for -stable.
Maybe there is some other odds it fixes and I'm not aware of. Linus?

Thanks,
//richard
Linus Walleij Feb. 17, 2019, 8:56 p.m. UTC | #3 
On Sun, Feb 17, 2019 at 7:18 PM Sasha Levin <sashal@kernel.org> wrote:
> On Thu, Feb 14, 2019 at 02:24:01PM +0100, Linus Walleij wrote:

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

> >

> >commit 1b7fc2c0069f3864a3dda15430b7aded31c0bfcc upstream.

> >

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

> >Signed-off-by: Richard Weinberger <richard@nod.at>

> >Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

>

> This one looks like a new feature that will also require changes to

> userspace. Is there actual breakage this fixes?


No let's drop it then, the problem I am investigating in OpenWrt and other
distributions (and code dumps) are backported patches: sometimes they
are obviously backported for features, sometimes obviously for fixing something
that was broken, sometimes it is just unclear to me why it has been
backported.

So I guess this was backported for features, so it can be dropped.

Thanks for helping out!
Linus Walleij
diff mbox series

Patch

diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
index 4d6ce4a2a4b6..3be28900bf37 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 ade4b3137a1d..b8b18d446a49 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