linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Carlos Maiolino <cmaiolino@redhat.com>
To: linux-fsdevel@vger.kernel.org
Cc: hch@lst.de, sandeen@redhat.com, david@fromorbit.com
Subject: [PATCH RFC 0/2] ->bmap interface retirement
Date: Wed,  5 Sep 2018 15:57:46 +0200	[thread overview]
Message-ID: <20180905135748.30098-1-cmaiolino@redhat.com> (raw)

Hi,

This series aims to deprecate the ->bmap interface, by using FIEMAP
infra-structure to handle FIBMAP calls. It doesn't change the FIBMAP api, so the
implementation is supposed to be transparent for users.

Although this patchset is working as I expected and it survived a few xfstests
runs, I have some questions about the way I implemented it that I'd like to
discuss, so, I'm tagging it as RFC.

The first patch is relatively simple, just replacing direct ->bmap calls by
bmap() function (as suggested by Dave Chinner), the second patch actually
changes bmap() function to call ->fiemap when available, falling back to ->bmap
when ->fiemap isn't implemented. So, in a (hopefully) not so far future, we can
get rid of the whole ->bmap interface.

These are the main points where I've doubts about:

I implemented it reutilizing the same infra-structure used by userspace FIEMAP
calls to avoid code duplication if possible, the problem is, to setup the struct
fiemap_extent_info to pass it to ->fiemap, I need to use (__force) casting, so
the compiler doesn't complain about the
"struct fiemap_extent __user *fi_extents_start" into it.
I saw a few other places in kernel doing something similar, so, that's why I
thought it would be ok to use such strategy.

I added a new FIEMAP flag (FIEMAP_KERNEL_FIBMAP), to signal the ->fiemap call
is supposed to be internal only and fiemap_fill_next_extent() can do a simple
memcpy() instead of a copy_to_user(). I added this flag to include/uapi, but I
suppose kernel only flags are not supposed to be in uapi, so, suggestions to
where I could place it are welcome.

Different filesystems will answer FIEMAP calls in slightly different ways. when
the requested range is somewhere in the middle of the extent, some filesystems
will set the map starting at the beginning of the extent (like ext4 for
example), and some (the ones based on iomap for example), will set the map
starting at the offset requested. So, to comply with both cases, bmap() checks
the logical offset returned by the filesystem, and take the appropriate action
if the filesystem returns the beginning of the extent, or not.

What you guys think?
Cheers.

Carlos Maiolino (2):
  fs: Replace direct ->bmap calls by bmap()
  Use fiemap internal infra-structure to handle FIBMAP

 fs/cachefiles/rdwr.c        |  5 ++---
 fs/ecryptfs/mmap.c          |  5 ++---
 fs/inode.c                  | 38 +++++++++++++++++++++++++++++++++++++-
 fs/ioctl.c                  | 25 +++++++++++++++++--------
 include/uapi/linux/fiemap.h |  1 +
 5 files changed, 59 insertions(+), 15 deletions(-)

-- 
2.14.4

             reply	other threads:[~2018-09-05 18:28 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-05 13:57 Carlos Maiolino [this message]
2018-09-05 13:57 ` [PATCH 1/2] fs: Replace direct ->bmap calls by bmap() Carlos Maiolino
2018-09-05 14:23   ` Eric Sandeen
2018-09-05 18:55     ` Christoph Hellwig
2018-09-06  8:04       ` Carlos Maiolino
2018-09-06  8:03     ` Carlos Maiolino
2018-09-05 13:57 ` [PATCH 2/2] Use fiemap internal infra-structure to handle FIBMAP Carlos Maiolino
2018-09-05 14:31   ` Matthew Wilcox
2018-09-05 18:56     ` Christoph Hellwig
2018-09-06  8:31       ` Carlos Maiolino
2018-09-10  7:50         ` Christoph Hellwig
2018-09-10 10:31           ` Carlos Maiolino
2018-09-26 13:34           ` Carlos Maiolino
2018-09-06  8:18     ` Carlos Maiolino
2018-09-05 14:40   ` Eric Sandeen
2018-09-06  8:12     ` Carlos Maiolino
2018-09-06 15:53       ` Eric Sandeen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180905135748.30098-1-cmaiolino@redhat.com \
    --to=cmaiolino@redhat.com \
    --cc=david@fromorbit.com \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=sandeen@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).