From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: linux-fsdevel@vger.kernel.org, hch@lst.de, adilger@dilger.ca,
jaegeuk@kernel.org, miklos@szeredi.hu, rpeterso@redhat.com,
linux-xfs@vger.kernel.org
Subject: Re: [PATCH 8/9] Use FIEMAP for FIBMAP calls
Date: Fri, 2 Aug 2019 08:29:02 -0700 [thread overview]
Message-ID: <20190802152902.GI7138@magnolia> (raw)
In-Reply-To: <20190802134816.usmauocewduggrjt@pegasus.maiolino.io>
On Fri, Aug 02, 2019 at 03:48:17PM +0200, Carlos Maiolino wrote:
> > > -#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC|FIEMAP_FLAG_XATTR)
> > > +#define EXT4_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC | \
> > > + FIEMAP_FLAG_XATTR| \
> > > + FIEMAP_KERNEL_FIBMAP)
> > >
> > > static int ext4_xattr_fiemap(struct inode *inode,
> > > struct fiemap_extent_info *fieinfo)
> > > @@ -5048,6 +5050,9 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo)
> > > if (ext4_has_inline_data(inode)) {
> > > int has_inline = 1;
> > >
> > > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> > > + return -EINVAL;
> >
> > Wouldn't the inline data case be caught by fiemap_bmap and turned into
> > -EINVAL?
>
> Yes, it does, but until ext4_fiemap() returns the extent with the INLINE flag,
> it does need to go through the whole fiemap mapping mechanism when we already
> know the result... So, instead of letting the ext4_fiemap() map the extent, just
> take the shortcut and return -EINVAL directly.
>
> The check in fiemap_bmap() is a 'safe measure' (if it does have other name I
> don't know :), but if the filesystem already knows it's gonna fall into an
> inline inode, taking the shortcut is better, isn't it?
I suppose so. Just wondering, that was all... :)
>
> > > + return 1;
> > > + return 0;
> > > +}
> > > +
> > > +static int bmap_fiemap(struct inode *inode, sector_t *block)
> > > +{
> > > + struct fiemap_extent_info fieinfo = { 0, };
> > > + struct fiemap_extent fextent;
> > > + u64 start = *block << inode->i_blkbits;
> > > + int error = -EINVAL;
> > > +
> > > + fextent.fe_logical = 0;
> > > + fextent.fe_physical = 0;
> > > + fieinfo.fi_extents_max = 1;
> > > + fieinfo.fi_extents_mapped = 0;
> > > + fieinfo.fi_cb_data = &fextent;
> > > + fieinfo.fi_start = start;
> > > + fieinfo.fi_len = 1 << inode->i_blkbits;
> > > + fieinfo.fi_cb = fiemap_fill_kernel_extent;
> > > + fieinfo.fi_flags = (FIEMAP_KERNEL_FIBMAP | FIEMAP_FLAG_SYNC);
> > > +
> > > + error = inode->i_op->fiemap(inode, &fieinfo);
> > > +
> > > + if (error)
> > > + return error;
> > > +
> > > + if (fieinfo.fi_flags & (FIEMAP_EXTENT_UNKNOWN |
> > > + FIEMAP_EXTENT_ENCODED |
> > > + FIEMAP_EXTENT_DATA_INLINE |
> > > + FIEMAP_EXTENT_UNWRITTEN |
> > > + FIEMAP_EXTENT_SHARED))
> > > + return -EINVAL;
> > > +
> > > + *block = (fextent.fe_physical +
> > > + (start - fextent.fe_logical)) >> inode->i_blkbits;
> > > +
> > > + return error;
> > > +}
> > > +
> > > /**
> > > * bmap - find a block number in a file
> > > * @inode: inode owning the block number being requested
> > > @@ -1591,10 +1663,15 @@ EXPORT_SYMBOL(iput);
> > > */
> > > int bmap(struct inode *inode, sector_t *block)
> > > {
> > > - if (!inode->i_mapping->a_ops->bmap)
> > > + if (inode->i_op->fiemap)
> > > + return bmap_fiemap(inode, block);
> > > +
> > > + if (inode->i_mapping->a_ops->bmap)
> > > + *block = inode->i_mapping->a_ops->bmap(inode->i_mapping,
> > > + *block);
> > > + else
> > > return -EINVAL;
> > >
> > > - *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
> > > return 0;
> > > }
> > > EXPORT_SYMBOL(bmap);
> > > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > > index d72696c222de..0759ac6e4c7e 100644
> > > --- a/fs/ioctl.c
> > > +++ b/fs/ioctl.c
> > > @@ -77,11 +77,8 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> > > return error;
> > > }
> > >
> > > -#define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC)
> > > -#define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED)
> > > -#define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE)
> > > -int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > > - u64 phys, u64 len, u32 flags)
> > > +static int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo,
> > > + u64 logical, u64 phys, u64 len, u32 flags)
> > > {
> > > struct fiemap_extent extent;
> > > struct fiemap_extent __user *dest = fieinfo->fi_cb_data;
> > > @@ -89,17 +86,17 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > > /* only count the extents */
> > > if (fieinfo->fi_extents_max == 0) {
> > > fieinfo->fi_extents_mapped++;
> > > - return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> > > + goto out;
> > > }
> > >
> > > if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
> > > return 1;
> > >
> > > - if (flags & SET_UNKNOWN_FLAGS)
> > > + if (flags & FIEMAP_EXTENT_DELALLOC)
> > > flags |= FIEMAP_EXTENT_UNKNOWN;
> > > - if (flags & SET_NO_UNMOUNTED_IO_FLAGS)
> > > + if (flags & FIEMAP_EXTENT_DATA_ENCRYPTED)
> > > flags |= FIEMAP_EXTENT_ENCODED;
> > > - if (flags & SET_NOT_ALIGNED_FLAGS)
> >
> > It's too bad that we lose the "not aligned" semantic meaning here.
>
> May you explain a bit better what you mean? We don't lose it, just the define
> goes away, the reason I dropped these defines is because the same flags are used
> in both functions, fiemap_fill_{user,kernel}_extent(), and I didn't think
> defining them on both places (or in fs.h) has any benefit here, so I opted to
> remove them.
Eh, I changed my mind. It's easy enough to tell which flags map to "No
umounted IO" from the code even if the #defines go away.
> >
> > > + if (flags & (FIEMAP_EXTENT_DATA_TAIL | FIEMAP_EXTENT_DATA_INLINE))
> > > flags |= FIEMAP_EXTENT_NOT_ALIGNED;
> >
> > Why doesn't this function just call fiemap_fill_kernel_extent to fill
> > out the onstack @extent structure? We've now implemented "fill out out
> > a struct fiemap_extent" twice.
>
> fiemap_fill_{user, kernel}_extent() have different purposes, and the big
> difference is one handles a userspace pointer memory and the other don't. IIRC
> the original proposal was some sort of sharing a single function, but then
> Christoph suggested a new design, using different functions as callbacks.
It's harder for me to tell when I don't have a branch containing the
final product to look at, but I'd have thought that _fill_kernel fills
out an in-kernel fiemap extent; and then _fill_user would declare one on
the stack, call _fill_kernel to set the fields, and then copy_to_user?
(But maybe the code already does this and I can't tell...)
>
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index b485190b7ecd..18a798e9076b 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -1113,6 +1113,11 @@ xfs_vn_fiemap(
> > > struct fiemap_extent_info *fieinfo)
> > > {
> > > int error;
> > > + struct xfs_inode *ip = XFS_I(inode);
> >
> > Would you mind fixing the indentation to match usual xfs style?
>
> Sure, will fix it
>
>
> >
> > > +
> > > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP)
> > > + if (xfs_is_reflink_inode(ip) || XFS_IS_REALTIME_INODE(ip))
> > > + return -EINVAL;
> >
> > The xfs part looks ok to me.
> >
> > --D
> >
>
> --
> Carlos
next prev parent reply other threads:[~2019-08-02 15:29 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-31 14:12 [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
2019-07-31 14:12 ` [PATCH 1/9] fs: Enable bmap() function to properly return errors Carlos Maiolino
2019-07-31 14:12 ` [PATCH 2/9] cachefiles: drop direct usage of ->bmap method Carlos Maiolino
2019-07-31 14:12 ` [PATCH 3/9] ecryptfs: drop direct calls to ->bmap Carlos Maiolino
2019-07-31 14:12 ` [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap Carlos Maiolino
2019-07-31 23:12 ` Darrick J. Wong
2019-08-02 9:19 ` Carlos Maiolino
2019-08-02 15:14 ` Darrick J. Wong
2019-08-05 10:27 ` Carlos Maiolino
2019-08-05 15:12 ` Darrick J. Wong
2019-08-06 5:38 ` Christoph Hellwig
2019-08-06 12:07 ` Carlos Maiolino
2019-08-06 14:48 ` Darrick J. Wong
2019-08-08 7:17 ` Carlos Maiolino
2019-08-06 12:02 ` Carlos Maiolino
2019-08-06 22:41 ` Luis Chamberlain
2019-08-07 14:42 ` Darrick J. Wong
2019-08-08 7:12 ` Carlos Maiolino
2019-08-08 18:53 ` Andreas Dilger
2019-08-19 10:10 ` Carlos Maiolino
2019-07-31 14:12 ` [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info Carlos Maiolino
2019-07-31 23:28 ` Darrick J. Wong
2019-08-02 9:51 ` Carlos Maiolino
2019-08-02 15:15 ` Darrick J. Wong
2019-08-05 9:40 ` Carlos Maiolino
2019-08-06 5:39 ` Christoph Hellwig
2019-07-31 14:12 ` [PATCH 6/9] iomap: Remove length and start fields from iomap_fiemap Carlos Maiolino
2019-07-31 23:24 ` Darrick J. Wong
2019-07-31 14:12 ` [PATCH 7/9] fiemap: Use a callback to fill fiemap extents Carlos Maiolino
2019-07-31 23:26 ` Darrick J. Wong
2019-07-31 14:12 ` [PATCH 8/9] Use FIEMAP for FIBMAP calls Carlos Maiolino
2019-07-31 14:12 ` Carlos Maiolino
2019-07-31 23:22 ` Darrick J. Wong
2019-07-31 23:31 ` Darrick J. Wong
2019-08-02 13:52 ` Carlos Maiolino
2019-08-06 5:41 ` Christoph Hellwig
2019-08-02 13:48 ` Carlos Maiolino
2019-08-02 15:29 ` Darrick J. Wong [this message]
2019-08-05 10:38 ` Carlos Maiolino
2019-08-06 5:46 ` Christoph Hellwig
2019-07-31 14:12 ` [PATCH 9/9] xfs: Get rid of ->bmap Carlos Maiolino
2019-07-31 23:30 ` Darrick J. Wong
2019-08-02 10:20 ` [PATCH 0/9 V4] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
2019-08-08 8:27 [PATCH 0/9 V5] " Carlos Maiolino
2019-08-08 8:27 ` [PATCH 8/9] Use FIEMAP for FIBMAP calls Carlos Maiolino
2019-08-08 8:27 ` Carlos Maiolino
2019-08-09 1:56 ` kbuild test robot
2019-08-14 11:18 ` Christoph Hellwig
2019-08-20 13:01 ` Carlos Maiolino
2019-08-29 7:15 ` Christoph Hellwig
2019-09-10 12:28 ` Carlos Maiolino
2019-09-16 15:58 ` Darrick J. Wong
2019-09-11 13:43 [PATCH 0/9 V6] New ->fiemap infrastructure and ->bmap removal Carlos Maiolino
2019-09-11 13:43 ` [PATCH 8/9] Use FIEMAP for FIBMAP calls Carlos Maiolino
2019-09-16 17:44 ` Darrick J. Wong
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=20190802152902.GI7138@magnolia \
--to=darrick.wong@oracle.com \
--cc=adilger@dilger.ca \
--cc=hch@lst.de \
--cc=jaegeuk@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=rpeterso@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).