| Message ID | 5703F32D02000078000E345C@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 05, 2016 at 09:17:33AM -0600, Jan Beulich wrote: > Commit 9c225f2655 ("vfs: atomic f_pos accesses as per POSIX") may have > gone a little too far: We've had a report of a deadlock of an > application accessing a /proc file through the same file descriptor > from multiple threads. While /proc files are regular ones, them (and > similarly others which are) often not being seekable really already > makes them deviate from how regular files would behave. > > The issue was specifically observed on /proc/xen/xenbus (which doesn't > exist in the upstream kernel), when an application's read blocks > (waiting for a watch to trigger) while the write that would satisfy > the read then waits for the position update mutex to be released. > > Since for non-seekable files the file position is kind of a strange > thing anyway, also don't enforce atomic position updates for them. > > (I do recognize that the application isn't really standard conforming, > as it should use multiple file descriptors from all I understand. But > it worked fine before that change, and so they claim the kernel to be > at fault.) We had already been through that discussion, IIRC, with that exact file. And the same question remains - why not have that flag cleared by xenbus ->open()? You are using very odd heuristics to catch files that have unusual locking requirements; there's no reason for those to never happen in combination with file being seekable. Sure, any such file will be able to clear he flag in its ->open(), but... so can xenbus. I'm not terribly opposed to the patch, but it really makes very little sense. You are adding an odd effect to nonseekable_open() instead of having it done directly in the affected ->open() instances (or adding a helper for them to call, for that matter). Seeing that the need to clear the damn thing is very rare, it makes more sense for a driver to document it than to rely upon your change of nonseekable_open() behaviour... -- 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 Tue, Apr 5, 2016 at 9:51 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > We had already been through that discussion, IIRC, with that exact file. > And the same question remains - why not have that flag cleared by xenbus > ->open()? You are using very odd heuristics to catch files that have > unusual locking requirements; So I actually much prefer Jan's patch and don't think his heuristics are very unusual at all. Instead of special-casing something lkike xenbus, Jan's patch says "if position isn't something meaningful, let's not waste time and effort on locking that makes no sense". So to me, Jan's patch is the generic and clean solution, and the fact that if fixes xenbus may be the reason the patch got written, but the patch makes sense to me on its own. But if you hate it, I guess a xenbus-specific hack would be fine, but I think it smells hackier than just saying "nonseekable also implies that you don't care about position locking". That statement not only describes the patch fairly well, it simply makes sense to me. Linus -- 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
--- 4.6-rc2/fs/open.c +++ 4.6-rc2-nonseekable-no-atomic-pos/fs/open.c @@ -1142,7 +1142,8 @@ EXPORT_SYMBOL(generic_file_open); */ int nonseekable_open(struct inode *inode, struct file *filp) { - filp->f_mode &= ~(FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE); + filp->f_mode &= ~(FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE | + FMODE_ATOMIC_POS); return 0; }
Commit 9c225f2655 ("vfs: atomic f_pos accesses as per POSIX") may have gone a little too far: We've had a report of a deadlock of an application accessing a /proc file through the same file descriptor from multiple threads. While /proc files are regular ones, them (and similarly others which are) often not being seekable really already makes them deviate from how regular files would behave. The issue was specifically observed on /proc/xen/xenbus (which doesn't exist in the upstream kernel), when an application's read blocks (waiting for a watch to trigger) while the write that would satisfy the read then waits for the position update mutex to be released. Since for non-seekable files the file position is kind of a strange thing anyway, also don't enforce atomic position updates for them. (I do recognize that the application isn't really standard conforming, as it should use multiple file descriptors from all I understand. But it worked fine before that change, and so they claim the kernel to be at fault.) Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Linus Torvalds <torvalds@linux-foundation.org> --- v2: Add a paragraph to the description outlining the actual issue observed. --- fs/open.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) -- 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