On Thu, Aug 30, 2018 at 08:34:02PM -0500, Eric Sandeen wrote:

On 8/30/18 7:11 PM, Dave Chinner wrote:

quoted

On Thu, Aug 30, 2018 at 01:51:56PM -0500, Eric Sandeen wrote:

quoted

On 8/30/18 1:28 PM, Darrick J. Wong wrote:

quoted

On Thu, Aug 30, 2018 at 02:02:05PM -0400, Brian Foster wrote:

quoted

On Thu, Aug 30, 2018 at 11:35:46AM -0500, Eric Sandeen wrote:

quoted

On 8/30/18 11:36 AM, Christoph Hellwig wrote:

quoted

On Thu, Aug 30, 2018 at 11:31:40AM -0500, Eric Sandeen wrote:

quoted

That's no reason to uniquely disallow it for reflinked files, though;
the problem is universal.  It's true for fiemap as well.  So I'm not sure
that's an argument against the patch?

fiemap at least tells you an extent is shared, bmap does not.

yes, so bmap is clearly the wrong interface to use if you want to
write directly to a file's blocks.  But if you know enough to check
the fiemap shared flag, you know enough to not use fibmap for that purpose...

FWIW, this patch seems reasonable to me. To Christoph's point, I don't
think either interface really grants license to write to the underlying
blocks, so either way it's technically being abused for this purpose.
Unless there's a clear way to return an error for a particular type of
file, I think it's reasonable behavior for fibmap to expose the data it
supports (i.e., block maps) and drop the data it doesn't (reflink
state).

But shared block status isn't something that can be dropped lightly.  If
you write to a shared block without realizing it, you'll corrupt every
other file that shares the block.

But there is no circumstance under which it is safe to write to a mapped
block no matter how you mapped it, tbh.

<sigh>

That's what all the break_layouts() code in XFS provides. It's a
mechanism for applications to prevent the block layout from changing
unexpected until they - the layout lease owner - give up their
exclusive access to the file layout.

quoted

Seriously, this has been talked about so much in the past year or
two in the context of DAX, RDMA, get_user_pages() races in direct
IO, etc. it pains me to see this discussion rehashing it all over
again.

We want applications to do what they need to do safely.  FIBMAP is
unsafe and, worse, it's unfixable. We need to get apps to move away
from it to something is actualayl safe.

quoted

Adding a file lease interface to block 3rd party changes to the
file layout until the app releases the lease is a safe way
of allowing userspace apps to use FIEMAP to map and identify
file extents they can write directly to if they need to.

IOWs, we need to get the FL_LAYOUT flag out into the external file
lease interface (IIRC Dan Williams posted patches for this a while
back) and get these "FIBMAP + write()" apps to use "FL_LAYOUT,
fsync(), FIEMAP, write(), ~FL_LAYOUT".

We need to make FIBMAP go away by providing a safer, more robust
solution to the problem people are trying to solve.

Sure.  I get it that it's not a great interface.  I get it that there
are better ways.  When those methods are available, we should explicitly
deprecate FIBMAP.

But until then I can't understand why we'd intentionally break an
otherwise reasonably functional interface in subtle and undetectable
ways for certain classes of files.

Because that's better than subtle, silent and undetectable data
corruption of certain classes of files in certain common FIBMAP use
cases.

We /could/ FIBMAP a reflinked file
exactly as well as we can FIBMAP a non-reflinked file, but we choose
not to.  This choice creates unnecessary problems for existing apps.

Yes, we /could/. But we don't, because because of the typical use
case of FIBMAP which is to map files so that IO (read and/or write)
can be done directly to the block device without going through the
filesystem. For normal files this is /relatively/ safe if the
correct precautions are taken. For a shared extent, writing is
*never* safe.

We don't know what users of FIBMAP are going to do with the block
map they get, but we *know* there are apps that use it to write
direct to block devices and we *know* that this will cause _silent_
data corruption and/or exposure of privileged information (shared
data can have different owners at different permission levels) when
it occurs.

That's what we're stopping by not exposing shared extents via FIBMAP
- applications that are careful and try to do everything as safely
and correctly as possible will still get it wrong and have no
indication that they've just screwed something up badly.

Until we deprecate the FIBMAP interface, until there is a better way,
I think we should make it as predictable and complete as
we can, not cripple it intentionally.

Crippling it is the lesser of two evils, unfortunately. I don't like
it either, but I like the idea of silent data corruption or data
exposure vectors even less.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com