Re: [PATCH v3] introduce sys_syncfs to sync a single file system

On Mon, 14 Mar 2011 02:56:52 +0100 (CET)
"Indan Zupancic" [off-list ref] wrote:

On Sat, March 12, 2011 18:32, Greg KH wrote:

quoted

On Fri, Mar 11, 2011 at 08:10:01PM -0600, Jonathan Nieder wrote:

quoted

Indan Zupancic wrote:

quoted

I'm not pushing for any official convention, just what seems good taste.

In cases like this, conventions (consistency and best practices) are
very important.

quoted

Less code added, less bloat. Architecture independent, no need to update
all system call tables everywhere (all archs, libc versions and strace).
Two files changed, instead of 7 (which only hooks up x86).

Thanks for explaining.  Those do seem like good reasons to use a ioctl
instead of a new syscall.

No, make it a syscall, it's more obvious and will be documented much
better.

There is no such guarantee. Everyone seems to want to add this new syncfs,
but it's not even defined what it does. "Same as sync, but only on one fs"
is IMHO not good enough, because sync's behaviour is pretty badly documented,
and that's a system call. The sync_file_range argument effects are quite
well defined, on the other hand, unlike sync behaviour. You're right for
ioctls though.

I think the semantics of sync are easily enough defined, even if
they're not well-defined in the documentation: all data which was dirty
at the time sync() was called will be written back and accessible when
the sync() returns.

I do agree that this should be a standalone syscall, not grafted into
sync_file_range() or into an ioctl.


That being said, we have two similar-looking-but-quite-different "sync"
concepts in the kernel.  One is "sync for data integrity" and the other
is "sync to reduce the dirty memory load".  The latter is not a data
integrity thing - it is a resource management thing.

There might one day be a requirement to be able to initiate a
resource-management-style writeback against a whole filesystem.  When
that happens, we'll regret not having added a "mode" argument to
sys_syncfs().

Or maybe not - given that we're syncing the entire fs and that
sync_filesystem() does the two-pass "write for data cleaning then write
for data integrity" thing, it could be that a syncfs-for-data-cleaning
operation has little performance benefit over a
syncfs-for-data-integrity operation.

On Mon, 2011-03-14 at 13:10 -0700, Andrew Morton wrote:

There might one day be a requirement to be able to initiate a
resource-management-style writeback against a whole filesystem.  When
that happens, we'll regret not having added a "mode" argument to
sys_syncfs().

I think Indan is right about an additional argument which could be used
for future extensions, what is the problem adding it?

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

--
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 Mon, Mar 14, 2011 at 01:10:42PM -0700, Andrew Morton wrote:

There might one day be a requirement to be able to initiate a
resource-management-style writeback against a whole filesystem.  When
that happens, we'll regret not having added a "mode" argument to
sys_syncfs().

I'm a bit nervous about exposing WB_SYNC_NONE to userspace, because
its semantics are *definitely* hard to describe.  For example, at the
moment if you do a WB_SYNC_NONE writeback, the writeback code will
clamp the amount of data written back for each inode to
MAX_WRITEBACK_PAGES (1024) pages.  Do we want to document that?
Probably not!  But if we don't document it, what can userspace expect?

If you just issue a writeback_inodes_sb(), it's not the case that it
will start a process that will eventually write out everything (i.e.,
it's not the equivalent of a non-blocking data integrity sync).  It
just means, "write out some stuff".

I could imagine userspace wanting to start a non-blocking writeout of
all data blocking pages, and which doesn't cause queue flush / barrier
requests.  (i.e., a non-blocking-non-barrier-issuing-but-otherwise-a-
data-integrity writeback) But that's not something that the current
writeback machinery can do easily, at least not today.

It wouldn't hurt to have a "flags" field which we could expand later

--- but that can lead to portability headaches for userspace programs

that don't know whether a particular kernel is going to support a
particular flag or not.  So it's certainly not a panacea.

	   	   	    	 	       - Ted

On Mon, 14 Mar 2011 17:11:19 -0400
"Ted Ts'o" [off-list ref] wrote:

On Mon, Mar 14, 2011 at 01:10:42PM -0700, Andrew Morton wrote:

quoted

There might one day be a requirement to be able to initiate a
resource-management-style writeback against a whole filesystem.  When
that happens, we'll regret not having added a "mode" argument to
sys_syncfs().

I'm a bit nervous about exposing WB_SYNC_NONE to userspace, because
its semantics are *definitely* hard to describe.  For example, at the
moment if you do a WB_SYNC_NONE writeback, the writeback code will
clamp the amount of data written back for each inode to
MAX_WRITEBACK_PAGES (1024) pages.

Wha?  It does?  When did that get broken?

 Do we want to document that?
Probably not!  But if we don't document it, what can userspace expect?

If you just issue a writeback_inodes_sb(), it's not the case that it
will start a process that will eventually write out everything (i.e.,
it's not the equivalent of a non-blocking data integrity sync).  It
just means, "write out some stuff".

I could imagine userspace wanting to start a non-blocking writeout of
all data blocking pages, and which doesn't cause queue flush / barrier
requests.  (i.e., a non-blocking-non-barrier-issuing-but-otherwise-a-
data-integrity writeback) But that's not something that the current
writeback machinery can do easily, at least not today.

Well.  Current implementation shortcomings don't carry a lot of weight
when designing a permanent interface.

I don't see a need to add an arg to syncfs() really.  But we should
demonstrate that we've thought about it ;)

On Monday 14 March 2011 22:11:19 Ted Ts'o wrote:

I think adding an unused flags argument can't hurt.

We could be fancy and ignore half the bits but bail out on the other
half with -EINVAL. That would make it possible to add both compatible
(default being full sync on old kernels) and incompatible (getting
rejected on old kernels) flags.

	Arnd