diff mbox series

exportfs: handle CONFIG_EXPORTFS=m also

Message ID 20231026192830.21288-1-rdunlap@infradead.org
State New
Headers
Series exportfs: handle CONFIG_EXPORTFS=m also |

Commit Message

Randy Dunlap Oct. 26, 2023, 7:28 p.m. UTC 
  When CONFIG_EXPORTFS=m, there are multiple build errors due to
the header <linux/exportfs.h> not handling the =m setting correctly.
Change the header file to check for CONFIG_EXPORTFS enabled at all
instead of just set =y.

Fixes: dfaf653dc415 ("exportfs: make ->encode_fh() a mandatory method for NFS export")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: linux-nfs@vger.kernel.org
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org

---
 include/linux/exportfs.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Amir Goldstein Oct. 26, 2023, 7:46 p.m. UTC | #1 
On Thu, Oct 26, 2023 at 10:28 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> When CONFIG_EXPORTFS=m, there are multiple build errors due to
> the header <linux/exportfs.h> not handling the =m setting correctly.
> Change the header file to check for CONFIG_EXPORTFS enabled at all
> instead of just set =y.
>
> Fixes: dfaf653dc415 ("exportfs: make ->encode_fh() a mandatory method for NFS export")
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: linux-nfs@vger.kernel.org
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
>
> ---
>  include/linux/exportfs.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff -- a/include/linux/exportfs.h b/include/linux/exportfs.h
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -314,7 +314,7 @@ extern struct dentry *exportfs_decode_fh
>  /*
>   * Generic helpers for filesystems.
>   */
> -#ifdef CONFIG_EXPORTFS
> +#if IS_ENABLED(CONFIG_EXPORTFS)
>  int generic_encode_ino32_fh(struct inode *inode, __u32 *fh, int *max_len,
>                             struct inode *parent);
>  #else

Thanks for the fix, but Arnd reported that this fix could cause a link
issue in some configuration - I did not verify.

I would much rather turn EXPORTFS into a bool config
and avoid the unneeded build test matrix.

See comparison to the amount of code of EXPORTFS
to BUFFER_HEAD and FS_IOMAP code which are bool:

~/src/linux$ wc -l fs/exportfs/*.c
636 fs/exportfs/expfs.c

~/src/linux$ wc -l fs/buffer.c fs/mpage.c
  3164 fs/buffer.c
   685 fs/mpage.c
  3849 total

~/src/linux$ wc -l fs/iomap/*.c
 2002 fs/iomap/buffered-io.c
  754 fs/iomap/direct-io.c
  124 fs/iomap/fiemap.c
   97 fs/iomap/iter.c
  104 fs/iomap/seek.c
  195 fs/iomap/swapfile.c
   13 fs/iomap/trace.c
 3289 total

Thanks,
Amir.
Christoph Hellwig Oct. 27, 2023, 6:01 a.m. UTC | #2 
On Thu, Oct 26, 2023 at 10:46:06PM +0300, Amir Goldstein wrote:
> I would much rather turn EXPORTFS into a bool config
> and avoid the unneeded build test matrix.

Yes.  Especially given that the defaul on open by handle syscalls
require it anyway.
Amir Goldstein Oct. 27, 2023, 6:11 a.m. UTC | #3 
On Fri, Oct 27, 2023 at 9:01 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Oct 26, 2023 at 10:46:06PM +0300, Amir Goldstein wrote:
> > I would much rather turn EXPORTFS into a bool config
> > and avoid the unneeded build test matrix.
>
> Yes.  Especially given that the defaul on open by handle syscalls
> require it anyway.

Note that those syscalls depend on CONFIG_FHANDLE and the latter
selects EXPORTFS.

Nevertheless, the EXPORTFS=m config seems useless.
I will send a patch to change it.

The bigger issue is that so many of the filesystems that use the
generic export ops do not select EXPORTFS, so it's easier to
leave the generic helper in libfs.c as Arnd suggested.

Thanks,
Amir.
Christoph Hellwig Oct. 27, 2023, 7:28 a.m. UTC | #4 
On Fri, Oct 27, 2023 at 09:11:57AM +0300, Amir Goldstein wrote:
> On Fri, Oct 27, 2023 at 9:01 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Thu, Oct 26, 2023 at 10:46:06PM +0300, Amir Goldstein wrote:
> > > I would much rather turn EXPORTFS into a bool config
> > > and avoid the unneeded build test matrix.
> >
> > Yes.  Especially given that the defaul on open by handle syscalls
> > require it anyway.
> 
> Note that those syscalls depend on CONFIG_FHANDLE and the latter
> selects EXPORTFS.

Yes, this means that for all somewhat sane configfs exportfs if always
built in anyway.  And for the ones where it isn't because people
are concerned about micro-optimizing kernel size, nfsd is unlikely
to be built in either.

> The bigger issue is that so many of the filesystems that use the
> generic export ops do not select EXPORTFS, so it's easier to
> leave the generic helper in libfs.c as Arnd suggested.

Agreed.
diff mbox series

Patch

diff -- a/include/linux/exportfs.h b/include/linux/exportfs.h
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -314,7 +314,7 @@  extern struct dentry *exportfs_decode_fh
 /*
  * Generic helpers for filesystems.
  */
-#ifdef CONFIG_EXPORTFS
+#if IS_ENABLED(CONFIG_EXPORTFS)
 int generic_encode_ino32_fh(struct inode *inode, __u32 *fh, int *max_len,
 			    struct inode *parent);
 #else