Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Andreas Dilger <adilger@dilger.ca>
To: Carlos Maiolino <cmaiolino@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Eric Sandeen <sandeen@redhat.com>,
	david@fromorbit.com
Subject: Re: [PATCH 09/10 V2] Use FIEMAP for FIBMAP calls
Date: Fri, 8 Feb 2019 14:03:12 -0700
Message-ID: <1CBE2C1E-8895-400F-913D-CC79BC1A0438@dilger.ca> (raw)
In-Reply-To: <20190208103617.dlhbal7xwglgujdu@hades.usersys.redhat.com>

[-- Attachment #1: Type: text/plain, Size: 3718 bytes --]

On Feb 8, 2019, at 3:36 AM, Carlos Maiolino <cmaiolino@redhat.com> wrote:
> 
> On Fri, Feb 08, 2019 at 09:46:12AM +0100, Christoph Hellwig wrote:
>> On Thu, Feb 07, 2019 at 02:25:01PM -0700, Andreas Dilger wrote:
>>> Do we really need to be this way, about reserving a single flag for Lustre,
>>> which will likely also be useful for other filesystems?  It's not like
>>> Lustre is some closed-source binary module for which we need to make life
>>> difficult, it is used by many thousands of the largest computers at labs
>>> and universities and companies around the world.  We are working to clean
>>> up the code outside the staging tree and resubmit it.  Not reserving a flag
>>> just means we will continue to use random values in Lustre before it can
>>> be merged, which will make life harder when we try to merge again.
>> 
>> No, it is available in source, but otherwise just as bad.  And we generally
>> only define APIs for in-kernel usage.
>> 
>> If we can come up with a good API for in-kernel filesystems we can do
>> that, otherwise hell no.  And staging for that matter qualifies as out
>> of tree.
>> 
>> That being said I'm really worried about these FIEMAP extensions as
>> userspace has no business poking into details of the placement (vs
>> just the layout).
> 
> I tend to say that identifying on which device an extent is is better than
> simply saying 'it maps to physical blocks X-Z, but it's your problem to identify
> which device X-Z belongs to'.
> 
>> But all that belongs into a separate dicussion instead of dragging down
>> this series where it does not belong at all.
> 
> Agreed, but now I'm on a kind of dead-end :P
> 
> Darrick's concerns are valid, regarding letting currently unsupported
> filesystems to suddenly allow FIBMAP calls,

I don't think there is a huge danger of people suddenly moving to use LILO
on f2fs or Btrfs with new kernels.  In most cases, it _would_ just work,
but the FIBMAP->FIEMAP layer needs to check for FIEMAP_FLAG_NOT_ALIGNED,
FIEMAP_FLAG_ENCODED, and FIEMAP_FLAG_DEVICE flags that would make this
unsuitable for booting.

> but on the other hand, his proposed
> solution, which is also valid, requires a new discussion/patchset to discuss an
> improvement of the FIEMAP infra-structure, and 'fix' the problem mentioned.
> Using a flag to identify FIBMAP calls has been rejected. So, I'd accept
> suggestions on how to move this patch forward, without requiring the
> improvements suggested by Darrick, and, without using a flag to tag FIBMAP
> calls, as suggested by me, I'm kind of running out of ideas by now :(

I think Darrick was against a flag like "FIEMAP_FLAG_FIBMAP" because it could
be specified from userspace, and it is a bit ugly and has no other value than
preventing FIBMAP from working on filesystems that don't support it today.

As Christoph mentioned, such a flag could be OK as long as it is masked from
userspace in the top-level ioctl_fiemap() handler (though to be honest, there
is no benefit for a userspace app to set this flag, it would just increase the
chance the ioctl(FIEMAP) call will fail).

     #define FIEMAP_FLAG_FIBMAP 0x80000000

Filesystems that don't want FIBMAP to work at all should return -ENOTTY from
their ->fiemap() handler.

That said, there is *still* a need for fe_device checking in ioctl_fibmap(),
because for filesystems that allow both FIEMAP and FIBMAP (i.e. the most common
ones like ext4 and XFS) there may still be reasons for FIBMAP to fail for some
files if they are unsuitable (e.g. data stored on multiple devices). That isn't
something that the filesystems should be checking themselves.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

  reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-05  9:17 [PATCH 00/10 V2] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
2018-12-05  9:17 ` [PATCH 01/10] fs: Enable bmap() function to properly return errors Carlos Maiolino
2018-12-05  9:17 ` [PATCH 02/10] cachefiles: drop direct usage of ->bmap method Carlos Maiolino
2018-12-05  9:17 ` [PATCH 03/10] ecryptfs: drop direct calls to ->bmap Carlos Maiolino
2018-12-05  9:17 ` [PATCH 04/10 V2] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap Carlos Maiolino
2019-01-14 16:49   ` Christoph Hellwig
2019-02-04 11:34     ` Carlos Maiolino
2018-12-05  9:17 ` [PATCH 05/10] fs: Move start and length fiemap fields into fiemap_extent_info Carlos Maiolino
2019-01-14 16:50   ` Christoph Hellwig
2018-12-05  9:17 ` [PATCH 06/10] iomap: Remove length and start fields from iomap_fiemap Carlos Maiolino
2019-01-14 16:51   ` Christoph Hellwig
2018-12-05  9:17 ` [PATCH 07/10] fs: Use a void pointer to store fiemap_extent Carlos Maiolino
2019-01-14 16:53   ` Christoph Hellwig
2018-12-05  9:17 ` [PATCH 08/10 V2] fiemap: Use a callback to fill fiemap extents Carlos Maiolino
2019-01-14 16:53   ` Christoph Hellwig
2018-12-05  9:17 ` [PATCH 09/10 V2] Use FIEMAP for FIBMAP calls Carlos Maiolino
2018-12-05 17:36   ` Darrick J. Wong
2018-12-07  9:09     ` Carlos Maiolino
2018-12-07 20:14       ` Andreas Dilger
2019-02-04 15:11     ` Carlos Maiolino
2019-02-04 18:27       ` Darrick J. Wong
2019-02-06 13:37         ` Carlos Maiolino
2019-02-06 20:44           ` Darrick J. Wong
2019-02-06 21:13             ` Andreas Dilger
2019-02-07  9:52               ` Carlos Maiolino
2019-02-08  8:43                 ` Christoph Hellwig
2019-02-11 12:57                   ` Christoph Hellwig
2019-02-11 16:21                     ` Carlos Maiolino
2019-02-11 16:48                       ` Christoph Hellwig
2019-02-07 11:59             ` Carlos Maiolino
2019-02-07 17:02               ` Darrick J. Wong
2019-02-07 21:25                 ` Andreas Dilger
2019-02-08  8:46                   ` Christoph Hellwig
2019-02-08 10:36                     ` Carlos Maiolino
2019-02-08 21:03                       ` Andreas Dilger [this message]
2019-02-08  9:08                   ` Carlos Maiolino
2019-02-08  9:03                 ` Carlos Maiolino
2019-02-07 12:36             ` Carlos Maiolino
2019-02-07 18:16               ` Darrick J. Wong
2019-02-08  8:58                 ` Carlos Maiolino
2019-02-06 21:04           ` Andreas Dilger
2019-01-14 16:56   ` Christoph Hellwig
2019-02-05  9:56     ` Carlos Maiolino
2019-02-05 18:25       ` Christoph Hellwig
2019-02-06  9:50         ` Carlos Maiolino
2018-12-05  9:17 ` [PATCH 10/10] xfs: Get rid of ->bmap Carlos Maiolino
2018-12-05 17:37   ` Darrick J. Wong
2018-12-06 13:06     ` Carlos Maiolino
2018-12-06 18:56 ` [PATCH 00/10 V2] New ->fiemap infrastructure and ->bmap removal Andreas Grünbacher
2018-12-07  9:34   ` Carlos Maiolino
2019-01-14 16:50     ` Christoph Hellwig
2019-01-14 17:56       ` Andreas Grünbacher
2019-01-14 17:58         ` Christoph Hellwig

Reply instructions:

You may reply publically 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=1CBE2C1E-8895-400F-913D-CC79BC1A0438@dilger.ca \
    --to=adilger@dilger.ca \
    --cc=cmaiolino@redhat.com \
    --cc=darrick.wong@oracle.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

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org linux-fsdevel@archiver.kernel.org
	public-inbox-index linux-fsdevel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox