Re: [fuse-devel] [PATCH 4/6] fs/fuse: support compiling out splice

[josh]

On Mon, Nov 24, 2014 at 11:34:12AM -0800, Greg KH wrote:
> On Mon, Nov 24, 2014 at 08:05:10AM -0800, Josh Triplett wrote:
> > On Mon, Nov 24, 2014 at 10:49:31AM +0100, Pieter Smith wrote:
> > > On Sun, Nov 23, 2014 at 03:23:02PM -0800, Josh Triplett wrote:
> > > > On Sun, Nov 23, 2014 at 11:29:08PM +0100, Richard Weinberger wrote:
> > > > > On Sun, Nov 23, 2014 at 3:20 PM, Pieter Smith <pieter@xxxxxxxxxx> wrote:
> > > > > > To implement splice support, fs/fuse makes use of nosteal_pipe_buf_ops. This
> > > > > > struct is exported by fs/splice. The goal of the larger patch set is to
> > > > > > completely compile out fs/splice, so uses of the exported struct need to be
> > > > > > compiled out along with fs/splice.
> > > > > >
> > > > > > This patch therefore compiles out splice support in fs/fuse when
> > > > > > CONFIG_SYSCALL_SPLICE is undefined.
> > > > > >
> > > > > > Signed-off-by: Pieter Smith <pieter@xxxxxxxxxx>
> > > > > > ---
> > > > > >  fs/fuse/dev.c      | 4 ++--
> > > > > >  include/linux/fs.h | 6 ++++++
> > > > > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > > > > > index ca88731..f8f92a4 100644
> > > > > > --- a/fs/fuse/dev.c
> > > > > > +++ b/fs/fuse/dev.c
> > > > > > @@ -1291,7 +1291,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, const struct iovec *iov,
> > > > > >         return fuse_dev_do_read(fc, file, &cs, iov_length(iov, nr_segs));
> > > > > >  }
> > > > > >
> > > > > > -static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
> > > > > > +static ssize_t __maybe_unused fuse_dev_splice_read(struct file *in, loff_t *ppos,
> > > > > >                                     struct pipe_inode_info *pipe,
> > > > > >                                     size_t len, unsigned int flags)
> > > > > >  {
> > > > > > @@ -2144,7 +2144,7 @@ const struct file_operations fuse_dev_operations = {
> > > > > >         .llseek         = no_llseek,
> > > > > >         .read           = do_sync_read,
> > > > > >         .aio_read       = fuse_dev_read,
> > > > > > -       .splice_read    = fuse_dev_splice_read,
> > > > > > +       .splice_read    = __splice_p(fuse_dev_splice_read),
> > > > > >         .write          = do_sync_write,
> > > > > >         .aio_write      = fuse_dev_write,
> > > > > >         .splice_write   = fuse_dev_splice_write,
> > > > > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > > > > index a957d43..04c0975 100644
> > > > > > --- a/include/linux/fs.h
> > > > > > +++ b/include/linux/fs.h
> > > > > > @@ -2443,6 +2443,12 @@ extern int blkdev_fsync(struct file *filp, loff_t start, loff_t end,
> > > > > >                         int datasync);
> > > > > >  extern void block_sync_page(struct page *page);
> > > > > >
> > > > > > +#ifdef CONFIG_SYSCALL_SPLICE
> > > > > > +#define __splice_p(x) x
> > > > > > +#else
> > > > > > +#define __splice_p(x) NULL
> > > > > > +#endif
> > > > > > +
> > > > > 
> > > > > This needs to go into a different patch.
> > > > > One logical change per patch please. :-)
> > > > 
> > > > Easy enough to merge this one into the patch introducing
> > > > CONFIG_SYSCALL_SPLICE, then.
> > > > 
> > > > - Josh Triplett
> > > 
> > > The patch introducing CONFIG_SYSCALL_SPLICE (PATCH 3) only compiles out the
> > > syscalls. PATCH 6 on the other hand, compiles out fs/splice.c. This patch
> > > allows fs/fuse to be compiled when fs/splice.c is compiled out. If I am to
> > > squash it, it would be logical to include it in PATCH 6, not 3.
> > 
> > The suggestion wasn't to move the fs/fuse/dev.c bits; those should
> > definitely stay in this patch.  The suggestion was just to move the bit
> > of the patch defining __splice_p from this patch to patch 3.  (Note that
> > you need to define it before you use it, so it can't go in patch 6.)
> 
> I would, again, argue that stuff like __splice_p() not be implemented at
> all please.  It will only cause a huge proliferation of stuff like this
> that will not make any sense, and only cause a trivial, if any, amount
> of code savings.
> 
> I thought you were going to not do this type of thing until you got the
> gcc optimizer working for function callbacks.

Compared to the previous patchset, there are now only two instances of
ifdefs outside of the splice code for this, and this is one of them.  In
this case, the issue is no longer about making the code for this
splice_read function disappear, but rather to eliminate a reference to a
bit of splice functionality (used *inside* the FUSE splice code) that
will not work without SPLICE_SYSCALL.

Would you prefer to see this specific case handled via an #ifdef in
fs/fuse/dev.c rather than introducing a __splice_p that people might be
inclined to propagate?  That'd be fine; the code could simply wrap
fuse_dev_splice_read in an #ifdef and have the #else define a NULL
fuse_dev_splice_read.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/