On Thu, Aug 30, 2018 at 02:47:09PM -0500, Eric Sandeen wrote: > On 8/30/18 2:39 PM, Brian Foster wrote: > >>> 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. > > >> 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
