Re: [PATCH v2] vfs: avoid atomic f_pos accesses for non-seekable files

[Al Viro <viro@ZenIV.linux.org.uk>]

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...