[00/10,V2] New ->fiemap infrastructure and ->bmap removal

Message

Carlos Maiolino Dec. 5, 2018, 9:17 a.m. UTC

Hi.

This is the second version of the complete series with the goal to remove ->bmap
interface completely, in lieu of FIEMAP.

This new version has been heavily modified in comparison with the first one,
based on comments of Christoph and Andreas. And has been simplified. My
apologies if I forgot to update anything based on previous discussion.

Patch 1-3 has no difference from the previous version, and Christoph's Reviewed-by
has been kept.

Patch 4 is a V2 of the previous set, with updates required by Christoph (a local
sector_t variable, and moving the patch earlier in the series.

Patches 5-9 are essentially the modification of ->fiemap and deprecation of
->bmap.

In this new version, I kept the current fiemap_extent_info structure, and moved
into it all the required data for use in both FIEMAP and FIBMAP, instead of
creating a new data structure as in the old patch set.
This way, basically no heavy modification is required on individual filesystems,
but the update of its fiemap methods, removing start and len arguments, once
they will be passed now into fiemap_extent_info.

Last patch, is the removal of ->bmap method into XFS, I decided to not add the
previous patch for Ext4 because it needs more ext4 internal knowledge, which I
don't have.

Comments are much appreciated.

Cheers

P.S.  I thought about merging patch 7 into patch 8, and, patch 6 into patch 5,
but I decided to leave it as-is by now, and get comments about the way it is,
and if people agree, I'll merge them.

Carlos Maiolino (10):
  fs: Enable bmap() function to properly return errors
  cachefiles: drop direct usage of ->bmap method.
  ecryptfs: drop direct calls to ->bmap
  fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
  fs: Move start and length fiemap fields into fiemap_extent_info
  iomap: Remove length and start fields from iomap_fiemap
  fs: Use a void pointer to store fiemap_extent
  fiemap: Use a callback to fill fiemap extents
  Use FIEMAP for FIBMAP calls
  xfs: Get rid of ->bmap

 drivers/md/md-bitmap.c |  16 +++---
 fs/bad_inode.c         |   3 +-
 fs/btrfs/inode.c       |   5 +-
 fs/cachefiles/rdwr.c   |  27 +++++-----
 fs/ecryptfs/mmap.c     |  16 +++---
 fs/ext2/ext2.h         |   3 +-
 fs/ext2/inode.c        |   6 +--
 fs/ext4/ext4.h         |   3 +-
 fs/ext4/extents.c      |   8 +--
 fs/f2fs/data.c         |   5 +-
 fs/f2fs/f2fs.h         |   3 +-
 fs/gfs2/inode.c        |   5 +-
 fs/hpfs/file.c         |   4 +-
 fs/inode.c             |  68 +++++++++++++++++++-----
 fs/ioctl.c             | 114 +++++++++++++++++++++++++++++------------
 fs/iomap.c             |   4 +-
 fs/jbd2/journal.c      |  22 +++++---
 fs/nilfs2/inode.c      |   5 +-
 fs/nilfs2/nilfs.h      |   3 +-
 fs/ocfs2/extent_map.c  |   5 +-
 fs/ocfs2/extent_map.h  |   3 +-
 fs/overlayfs/inode.c   |   5 +-
 fs/xfs/xfs_aops.c      |  24 ---------
 fs/xfs/xfs_iops.c      |  14 ++---
 fs/xfs/xfs_trace.h     |   1 -
 include/linux/fs.h     |  31 +++++++----
 include/linux/iomap.h  |   2 +-
 mm/page_io.c           |  11 ++--
 28 files changed, 248 insertions(+), 168 deletions(-)

Comments

Andreas Grünbacher Dec. 6, 2018, 6:56 p.m. UTC | #1

Hi,

Am Mi., 5. Dez. 2018 um 10:18 Uhr schrieb Carlos Maiolino
<[e-mail]>:
> This is the second version of the complete series with the goal to remove ->bmap
> interface completely, in lieu of FIEMAP.

I'm not thrilled by this approach. How about exposing the iomap
operations at the vfs layer (for example, in the super block) and
implementing bmap on top of that instead?

(I realize that xfs has separate iomap operations for xattrs, but that
is only used in its fiemap implementation.)

Thanks,
Andreas

Carlos Maiolino Dec. 7, 2018, 9:34 a.m. UTC | #2

On Thu, Dec 06, 2018 at 07:56:02PM +0100, Andreas Grünbacher wrote:
> Hi,
> 
> Am Mi., 5. Dez. 2018 um 10:18 Uhr schrieb Carlos Maiolino
> <[e-mail]>:
> > This is the second version of the complete series with the goal to remove ->bmap
> > interface completely, in lieu of FIEMAP.
> 
> I'm not thrilled by this approach. How about exposing the iomap
> operations at the vfs layer (for example, in the super block) and
> implementing bmap on top of that instead?
> 

Well, the idea is exactly to get rid of bmap, not reimplement it. We can use the
same operation for both cases (fiemap+fibmap), so I honestly don't see which
advantages would be by reimplementing it.

> (I realize that xfs has separate iomap operations for xattrs, but that
> is only used in its fiemap implementation.)
> 
> Thanks,
> Andreas

Christoph Hellwig Jan. 14, 2019, 4:50 p.m. UTC | #3

On Fri, Dec 07, 2018 at 10:34:29AM +0100, Carlos Maiolino wrote:
> On Thu, Dec 06, 2018 at 07:56:02PM +0100, Andreas Grünbacher wrote:
> > Hi,
> > 
> > Am Mi., 5. Dez. 2018 um 10:18 Uhr schrieb Carlos Maiolino
> > <[e-mail]>:
> > > This is the second version of the complete series with the goal to remove ->bmap
> > > interface completely, in lieu of FIEMAP.
> > 
> > I'm not thrilled by this approach. How about exposing the iomap
> > operations at the vfs layer (for example, in the super block) and
> > implementing bmap on top of that instead?
> > 
> 
> Well, the idea is exactly to get rid of bmap, not reimplement it. We can use the
> same operation for both cases (fiemap+fibmap), so I honestly don't see which
> advantages would be by reimplementing it.

Exactly.  iomap really is a possibly implementation.  Everytime we
exposed implementation details at the ops level that created horrible
abuses.  The most important still relevant example is
write_begin/write_end, which require fs specific locking but are
exposed in a way where we can't easily enforce that.

Andreas Grünbacher Jan. 14, 2019, 5:56 p.m. UTC | #4

Am Mo., 14. Jan. 2019 um 17:50 Uhr schrieb Christoph Hellwig <[e-mail]>:
> On Fri, Dec 07, 2018 at 10:34:29AM +0100, Carlos Maiolino wrote:
> > On Thu, Dec 06, 2018 at 07:56:02PM +0100, Andreas Grünbacher wrote:
> > > Hi,
> > >
> > > Am Mi., 5. Dez. 2018 um 10:18 Uhr schrieb Carlos Maiolino
> > > <[e-mail]>:
> > > > This is the second version of the complete series with the goal to remove ->bmap
> > > > interface completely, in lieu of FIEMAP.
> > >
> > > I'm not thrilled by this approach. How about exposing the iomap
> > > operations at the vfs layer (for example, in the super block) and
> > > implementing bmap on top of that instead?
> > >
> >
> > Well, the idea is exactly to get rid of bmap, not reimplement it. We can use the
> > same operation for both cases (fiemap+fibmap), so I honestly don't see which
> > advantages would be by reimplementing it.
>
> Exactly.  iomap really is a possibly implementation.  Everytime we
> exposed implementation details at the ops level that created horrible
> abuses.  The most important still relevant example is
> write_begin/write_end, which require fs specific locking but are
> exposed in a way where we can't easily enforce that.

Yes, locking. The fiemap_fill_cb callback hack still makes the fiemap
interface much uglier though. So couldn't the existing iop be used to
fill a kernel buffer in a way similar to what functions like
kernel_readv do? That would at least avoid wrecking an existing
interface.

Thanks,
Andreas

Christoph Hellwig Jan. 14, 2019, 5:58 p.m. UTC | #5

On Mon, Jan 14, 2019 at 06:56:16PM +0100, Andreas Grünbacher wrote:
> Yes, locking. The fiemap_fill_cb callback hack still makes the fiemap
> interface much uglier though. So couldn't the existing iop be used to
> fill a kernel buffer in a way similar to what functions like
> kernel_readv do? That would at least avoid wrecking an existing
> interface.

There is no file system visible change at all, the callback happens
all behind the back.  We could do a less extensible union based version,
but I see absolutely no upside in that.

set_fs as in kernel_readv needs to go away, so no new users should be
added.