Message ID | 5703E2E602000078000E3331@prv-mh.provo.novell.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Apr 5, 2016 at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote: > > 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 don't disagree with the patch at all, I'm just wondering exactly what the deadlock was, and would love to have that description in the commit message. > (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.) Oh, absolutely. Very much a regression. There are almost zero interesting applications that are "standard conforming", so if we used that as an excuse to not fix kernel issues, we'd never have to do any work. And we'd have no users ;) > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> Al, make that a "Acked-by:", although I do want to repeat that it would be lovely to hear what the app actually did to trigger the problem. Which /proc file is it that causes the read/write/lseek path to then presumably recurse into another read/write/lseek and cause a deadlock? Are there perhaps other cases we'd need to worry about? 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
>>> On 05.04.16 at 16:19, <torvalds@linux-foundation.org> wrote: > Which /proc file is it that causes the read/write/lseek path to then > presumably recurse into another read/write/lseek and cause a deadlock? > Are there perhaps other cases we'd need to worry about? It was /proc/xen/xenbus, which doesn't exist in the upstream kernel (it's replacement is a miscdev at /dev/xen/xenbus there), hence I didn't really consider it feasible to include that additional information in the commit message. If you're nevertheless interested in the details, I can try to reconstruct that from the report (I didn't observe the problem myself, it was just reasonably straightforward to find the cause of it), or I could point you at the (openSUSE) bug report. Jan -- 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 7:28 AM, Jan Beulich <JBeulich@suse.com> wrote: > > It was /proc/xen/xenbus, which doesn't exist in the upstream kernel > (it's replacement is a miscdev at /dev/xen/xenbus there), hence I > didn't really consider it feasible to include that additional information > in the commit message. Hmm. That might at least be worth mentioning. And it might explain why we haven't seen problems with that commit 9c225f2655 even though it goes back a few years. Regardless, I don't object to the patch at all, I'd just like a bit more background information. So if you can give even a fairly handwavy overview of what the xenbus locking deadlock was, that would be great. Thanks, 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. 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> Cc: Linus Torvalds <torvalds@linux-foundation.org> --- 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