linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Carlos Maiolino <cmaiolino@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Matthew Wilcox <willy@infradead.org>,
	linux-fsdevel@vger.kernel.org, sandeen@redhat.com,
	david@fromorbit.com
Subject: Re: [PATCH 2/2] Use fiemap internal infra-structure to handle FIBMAP
Date: Wed, 26 Sep 2018 15:34:39 +0200	[thread overview]
Message-ID: <20180926133439.yjxksvjclbio4yyl@odin.usersys.redhat.com> (raw)
In-Reply-To: <20180910075039.GA14999@lst.de>

Hi Christoph.

On Mon, Sep 10, 2018 at 09:50:39AM +0200, Christoph Hellwig wrote:
> On Thu, Sep 06, 2018 at 10:31:05AM +0200, Carlos Maiolino wrote:
> > 
> > Sorry, I'm not 100% sure I followed your point here, do you mind to detail it a
> > bit?
> > Passing fiemap_fill_next_extent() as a pointer to what? ->fiemap() interface?
> 
> Yes.
> 
> > Sounds interesting, but doing this looks like I'll need to do what I was trying
> > to avoid from the beginning, which is the creating of a second struct
> > fiemap_extent_info, to be used in-lernel only, so, the last field doesn't need
> > to be tagged as __user. I'm ok with that though, I was just trying a way to
> > avoid adding unneeded data structures if possible, but looks like it ended up
> > not being a good approach :P
> 
> fieinfo is mostly used as an opaqueue cookie, so I think it should
> be possible to just pass a void pointer to fiemap and only let the 'filler'
> callback (that is fiemap_fill_next_extent or whatever a kernel caller
> passed) interpret it.
> 
> The only thing breaking this right now seems to be fi_flags, so maybe
> we just need to pass that explicitly as another argument to ->fiemap.
> 
> Something like:
> 
> typedef int (fiemap_fill_cb)(struct inode *inode, void *data, u64 logical,
> 		u64 phys, u64 len, u32 flags);
> 
> struct file_operations {
> 	...
> 	int (*fiemap)(struct inode *, unsigned int, fiemap_fill_cb,
> 			void *, u64 start, u64 len);
> 	...
> };

I've been playing around with this idea, and one thing that I've been having
problems with (at least that I'm not liking the end result), is the fact some
filesystems keep juggling with the *fiemap_extent_info passed to them. Like
ext4, for example, where it takes the pointer and passes it to its internal
functions or to generic_block_fiemap if that's the case.

I was wondering if, maybe, creating a new structure to embed the cookie, the
flags, and the fill callback wouldn't be better, I've been thinking it might be
easier to refactor the code needed to handle this new structure with all needed
data into it, instead of handling the callback, flags and the cookie by their
own. Maybe even create some helpers to handle the data into it.
What you think?


-- 
Carlos

  parent reply	other threads:[~2018-09-26 19:47 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-05 13:57 [PATCH RFC 0/2] ->bmap interface retirement Carlos Maiolino
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 [this message]
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=20180926133439.yjxksvjclbio4yyl@odin.usersys.redhat.com \
    --to=cmaiolino@redhat.com \
    --cc=david@fromorbit.com \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=sandeen@redhat.com \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH 2/2] Use fiemap internal infra-structure to handle FIBMAP' \
    /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

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).