| Message ID | 20190424071316.11967-1-kirr@nexedi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Hello up there, On Wed, Apr 24, 2019 at 04:06:10PM +0000, Sasha Levin wrote: > Hi, > > [This is an automated email] > > This commit has been processed because it contains a -stable tag. > The stable tag indicates that it's relevant for the following trees: 3.14+ > > The bot has tested the following trees: v5.0.9, v4.19.36, v4.14.113, v4.9.170, v4.4.178, v3.18.138. > > v5.0.9: Build failed! Errors: > fs/fuse/file.c:185:3: error: implicit declaration of function ‘stream_open’; did you mean ‘seq_open’? [-Werror=implicit-function-declaration] This patch needs "fs: stream_open - opener for stream-like files so that read and write can run simultaneously without deadlock" (10dce8af3422) as its dependency. It documents so in its commit message. That base dependency patch is being discussed here in stable context: https://lore.kernel.org/linux-fsdevel/20190424183012.GB3798@deco.navytux.spb.ru/ > v4.19.36: Failed to apply! Possible dependencies: > 6433b8998a21 ("fuse: add FOPEN_CACHE_DIR") > 88bc7d5097a1 ("fuse: add support for copy_file_range()") "fuse: add FOPEN_CACHE_DIR" added another nearby constant. The conflict with that patch should be trivially resolvable (just add FOPEN_STREAM irregardless that there is no FOPEN_CACHE_DIR in context). > v4.14.113: Failed to apply! Possible dependencies: > 3b7008b226f3 ("fuse: return -ECONNABORTED on /dev/fuse read after abort") > 6433b8998a21 ("fuse: add FOPEN_CACHE_DIR") > 88bc7d5097a1 ("fuse: add support for copy_file_range()") same. > v4.9.170: Failed to apply! Possible dependencies: > 3b7008b226f3 ("fuse: return -ECONNABORTED on /dev/fuse read after abort") > 6433b8998a21 ("fuse: add FOPEN_CACHE_DIR") > 88bc7d5097a1 ("fuse: add support for copy_file_range()") ----//---- > v4.4.178: Failed to apply! Possible dependencies: > 29433a2991fa ("fuse: get rid of fc->flags") > 3767e255b390 ("switch ->setxattr() to passing dentry and inode separately") > 60bcc88ad185 ("fuse: Add posix ACL support") > 6192269444eb ("introduce a parallel variant of ->iterate()") > 6433b8998a21 ("fuse: add FOPEN_CACHE_DIR") > 6b2553918d8b ("replace ->follow_link() with new method that could stay in RCU mode") > 703c73629f93 ("fuse: Use generic xattr ops") > 84e710da2a1d ("parallel lookups machinery, part 2") > 88bc7d5097a1 ("fuse: add support for copy_file_range()") > 9902af79c01a ("parallel lookups: actual switch to rwsem") > 9cf843e3f47c ("lookup_open(): lock the parent shared unless O_CREAT is given") > aa80deab33a8 ("namei: page_getlink() and page_follow_link_light() are the same thing") > cd3417c8fc95 ("kill free_page_put_link()") > ce23e6401334 ("->getxattr(): pass dentry and inode as separate arguments") > fceef393a538 ("switch ->get_link() to delayed_call, kill ->put_link()") similar > v3.18.138: Failed to apply! Possible dependencies: > 09561a53b50d ("lustre: use %p[dD]") > 29433a2991fa ("fuse: get rid of fc->flags") > 2b0143b5c986 ("VFS: normal filesystems (and lustre): d_inode() annotations") > 3767e255b390 ("switch ->setxattr() to passing dentry and inode separately") > 60bcc88ad185 ("fuse: Add posix ACL support") > 6192269444eb ("introduce a parallel variant of ->iterate()") > 6433b8998a21 ("fuse: add FOPEN_CACHE_DIR") > 680baacbca69 ("new ->follow_link() and ->put_link() calling conventions") > 6b2553918d8b ("replace ->follow_link() with new method that could stay in RCU mode") > 6e77137b363b ("don't pass nameidata to ->follow_link()") > 703c73629f93 ("fuse: Use generic xattr ops") > 84e710da2a1d ("parallel lookups machinery, part 2") > 88bc7d5097a1 ("fuse: add support for copy_file_range()") > 90e4fc8890da ("9p: don't bother with __getname() in ->follow_link()") > 9902af79c01a ("parallel lookups: actual switch to rwsem") > 9cf843e3f47c ("lookup_open(): lock the parent shared unless O_CREAT is given") > a06ae8609b3d ("coresight: add CoreSight core layer framework") > ce23e6401334 ("->getxattr(): pass dentry and inode as separate arguments") > dab363f938a5 ("Merge tag 'staging-3.19-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging") > fceef393a538 ("switch ->get_link() to delayed_call, kill ->put_link()") similar > How should we proceed with this patch? I have backported this patch to above those stable trees. You can pull the result from https://lab.nexedi.com/kirr/linux.git, branches: fopen_stream-5.0.y fopen_stream-4.19.y fopen_stream-4.14.y fopen_stream-4.4.y ( actually fixed deadlock on /proc/xen/xenbus as 581d21a2d02a was not backported to 4.4 ) fopen_stream-3.18.y ( actually fixed deadlock on /proc/xen/xenbus as 581d21a2d02a was not backported to 3.18 ) Hope it helps a bit, Kirill P.S. The fact that 4.4 and 3.18 versions of stream_open patch had to resolve xenbus conflict in a way that actually fixes /proc/xen/xenbus deadlock (introduced in 3.14) suggests that deadlock error messages produced by stream_open.cocci should indeed be considered by relevant maintainers including stable team...
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 06096b60f1df..44de96cb7871 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -178,7 +178,9 @@ void fuse_finish_open(struct inode *inode, struct file *file) if (!(ff->open_flags & FOPEN_KEEP_CACHE)) invalidate_inode_pages2(inode->i_mapping); - if (ff->open_flags & FOPEN_NONSEEKABLE) + if (ff->open_flags & FOPEN_STREAM) + stream_open(inode, file); + else if (ff->open_flags & FOPEN_NONSEEKABLE) nonseekable_open(inode, file); if (fc->atomic_o_trunc && (file->f_flags & O_TRUNC)) { struct fuse_inode *fi = get_fuse_inode(inode); diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index 2ac598614a8f..26abf0a571c7 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -229,11 +229,13 @@ struct fuse_file_lock { * FOPEN_KEEP_CACHE: don't invalidate the data cache on open * FOPEN_NONSEEKABLE: the file is not seekable * FOPEN_CACHE_DIR: allow caching this directory + * FOPEN_STREAM: the file is stream-like */ #define FOPEN_DIRECT_IO (1 << 0) #define FOPEN_KEEP_CACHE (1 << 1) #define FOPEN_NONSEEKABLE (1 << 2) #define FOPEN_CACHE_DIR (1 << 3) +#define FOPEN_STREAM (1 << 4) /** * INIT request/reply flags
Starting from 9c225f2655 (vfs: atomic f_pos accesses as per POSIX) files opened even via nonseekable_open gate read and write via lock and do not allow them to be run simultaneously. This can create read vs write deadlock if a filesystem is trying to implement a socket-like file which is intended to be simultaneously used for both read and write from filesystem client. See 10dce8af3422 ("fs: stream_open - opener for stream-like files so that read and write can run simultaneously without deadlock") for details and e.g. 581d21a2d0 (xenbus: fix deadlock on writes to /proc/xen/xenbus) for a similar deadlock example on /proc/xen/xenbus. To avoid such deadlock it was tempting to adjust fuse_finish_open to use stream_open instead of nonseekable_open on just FOPEN_NONSEEKABLE flags, but grepping through Debian codesearch shows users of FOPEN_NONSEEKABLE, and in particular GVFS which actually uses offset in its read and write handlers https://codesearch.debian.net/search?q=-%3Enonseekable+%3D https://gitlab.gnome.org/GNOME/gvfs/blob/1.40.0-6-gcbc54396/client/gvfsfusedaemon.c#L1080 https://gitlab.gnome.org/GNOME/gvfs/blob/1.40.0-6-gcbc54396/client/gvfsfusedaemon.c#L1247-1346 https://gitlab.gnome.org/GNOME/gvfs/blob/1.40.0-6-gcbc54396/client/gvfsfusedaemon.c#L1399-1481 so if we would do such a change it will break a real user. -> Add another flag (FOPEN_STREAM) for filesystem servers to indicate that the opened handler is having stream-like semantics; does not use file position and thus the kernel is free to issue simultaneous read and write request on opened file handle. This patch together with stream_open (10dce8af3422) should be added to stable kernels starting from v3.14+ (the kernel where 9c225f2655 first appeared). This will allow to patch OSSPD and other FUSE filesystems that provide stream-like files to return FOPEN_STREAM | FOPEN_NONSEEKABLE in open handler and this way avoid the deadlock on all kernel versions. This should work because fuse_finish_open ignores unknown open flags returned from a filesystem and so passing FOPEN_STREAM to a kernel that is not aware of this flag cannot hurt. In turn the kernel that is not aware of FOPEN_STREAM will be < v3.14 where just FOPEN_NONSEEKABLE is sufficient to implement streams without read vs write deadlock. Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Michael Kerrisk <mtk.manpages@gmail.com> Cc: Yongzhi Pan <panyongzhi@gmail.com> Cc: Jonathan Corbet <corbet@lwn.net> Cc: David Vrabel <david.vrabel@citrix.com> Cc: Juergen Gross <jgross@suse.com> Cc: Tejun Heo <tj@kernel.org> Cc: Kirill Tkhai <ktkhai@virtuozzo.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Christoph Hellwig <hch@lst.de> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Julia Lawall <Julia.Lawall@lip6.fr> Cc: Nikolaus Rath <Nikolaus@rath.org> Cc: Han-Wen Nienhuys <hanwen@google.com> Cc: stable@vger.kernel.org # v3.14+ Signed-off-by: Kirill Smelkov <kirr@nexedi.com> --- ( resending the same patch with updated description to reference stream_open that was landed to master as 10dce8af3422; also added cc stable ) fs/fuse/file.c | 4 +++- include/uapi/linux/fuse.h | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-)