From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zeniv.linux.org.uk ([195.92.253.2]:35304 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754155AbcDEQv3 (ORCPT ); Tue, 5 Apr 2016 12:51:29 -0400 Date: Tue, 5 Apr 2016 17:51:21 +0100 From: Al Viro To: Jan Beulich Cc: Linus Torvalds , linux-fsdevel Subject: Re: [PATCH v2] vfs: avoid atomic f_pos accesses for non-seekable files Message-ID: <20160405165121.GN17997@ZenIV.linux.org.uk> References: <5703E2E602000078000E3331@prv-mh.provo.novell.com> <5703E7AE02000078000E3397@prv-mh.provo.novell.com> <5703F32D02000078000E345C@prv-mh.provo.novell.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5703F32D02000078000E345C@prv-mh.provo.novell.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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...