On Thu, Aug 30, 2018 at 02:47:09PM -0500, Eric Sandeen wrote:

On 8/30/18 2:39 PM, Brian Foster wrote:

quoted

quoted

quoted

Frankly I think FIBMAP comes verrry
close to "this API is unfixably stupid and shouldn't be enabled for new
use cases and should go away some day".

Backing up, the interface isn't /that/ dumb, other than being limited to
32 bits.  Q: "Where is this logical file block physically located?"  A: "It is here."

Inefficient, sure.

quoted

quoted

So instead if anyone asks we'll just give them a successful response which
is indistinguishable from a hole.  :(

... but this seems to be the crux of the matter (IMO, at least). If we
can return -ENOTSUPP or whatever, then it can be made obvious that the
user either needs to use fiemap or avoid using reflinked files. ISTM
that what we do now is essentially report an incorrect bmap, which leads
to these subtle bug reports.

I haven't dug into the fibmap code.. does something prevent returning a
legitimate error code?

so ->bmap() returns a sector_t, which is a u64 and doesn't really
leave room for a negative error return.  But the ioctl interface 
stuffs that return into an int, and returns it to the user.
(note to self, wonder why it doesn't return -EOVERFLOW as needed).

Ah, so the syscall can return an error, but the internal interface
basically doesn't support error propagation from the fs callback.

I suppose ->bmap() could be changed to take a u64 in/out pointer like the
user interface does, and return 0 or -errno.  *Shrug* this seems like
a lot of work to avoid simply giving the user what they asked
for.  ;)

FWIW, another option might be to drop the ->bmap() callback on reflinked
inodes, which looks like it would trigger an -EINVAL to userspace. I
also don't know if that is worth whatever trouble might be required to
change function pointers like that, if it can even be done safely. I
guess my take boils down to that I think either returning an error (one
way or another) or accurate block map info is more sane than the current
behavior.

Brian

-Eric