[GIT PULL] nfsd changes for 5.18

[GIT PULL] nfsd changes for 5.18

[Chuck Lever III]

Hi Linus-

The following changes since commit 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3:

  Linux 5.17-rc6 (2022-02-27 14:36:33 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git tags/nfsd-5.18

for you to fetch changes up to 4fc5f5346592cdc91689455d83885b0af65d71b8:

  nfsd: fix using the correct variable for sizeof() (2022-03-20 12:49:38 -0400)

----------------------------------------------------------------
New features:
- NFSv3 support in NFSD is now always built
- Added NFSD support for the NFSv4 birth-time file attribute
- Added support for storing and displaying sockaddrs in trace points
- NFSD now recognizes RPC_AUTH_TLS probes

Performance improvements:
- Optimized the svc transport enqueuing mechanism
- Added micro-optimizations for the duplicate reply cache

Notable bug fixes:
- Allocation of the NFSD file cache hash table is more reliable

----------------------------------------------------------------
Amir Goldstein (1):
      nfsd: more robust allocation failure handling in nfsd_file_cache_init

Bill Wendling (1):
      nfsd: use correct format characters

Chuck Lever (22):
      NFSD: De-duplicate hash bucket indexing
      NFSD: Skip extra computation for RC_NOCACHE case
      NFSD: Streamline the rare "found" case
      tracing: Introduce helpers to safely handle dynamic-sized sockaddrs
      NFSD: Use __sockaddr field to store socket addresses
      NFSD: Remove NFSD_PROC_ARGS_* macros
      SUNRPC: Improve sockaddr handling in the svc_xprt_create_error trace point
      SUNRPC: Same as SVC_RQST_ENDPOINT, but without the xid
      SUNRPC: Record endpoint information in trace log
      SUNRPC: Remove the .svo_enqueue_xprt method
      SUNRPC: Merge svc_do_enqueue_xprt() into svc_enqueue_xprt()
      SUNRPC: Remove svo_shutdown method
      SUNRPC: Rename svc_create_xprt()
      SUNRPC: Rename svc_close_xprt()
      SUNRPC: Remove svc_shutdown_net()
      NFSD: Remove svc_serv_ops::svo_module
      NFSD: Move svc_serv_ops::svo_function into struct svc_serv
      SUNRPC: Teach server to recognize RPC_AUTH_TLS
      NFSD: Remove CONFIG_NFSD_V3
      arch: Remove references to CONFIG_NFSD_V3 in the default configs
      NFSD: Clean up _lm_ operation names
      NFSD: Fix nfsd_breaker_owns_lease() return values

Dai Ngo (1):
      fs/lock: documentation cleanup. Replace inode->i_lock with flc_lock.

Dan Carpenter (2):
      NFSD: prevent underflow in nfssvc_decode_writeargs()
      NFSD: prevent integer overflow on 32 bit systems

Jakob Koschel (1):
      nfsd: fix using the correct variable for sizeof()

Ondrej Valousek (1):
      nfsd: Add support for the birth time attribute

Steven Rostedt (Google) (1):
      tracing: Update print fmt check to handle new __get_sockaddr() macro

 Documentation/filesystems/locking.rst       |   6 +--
 arch/alpha/configs/defconfig                |   1 -
 arch/arm/configs/davinci_all_defconfig      |   1 -
 arch/arm/configs/ezx_defconfig              |   1 -
 arch/arm/configs/imote2_defconfig           |   1 -
 arch/arm/configs/integrator_defconfig       |   1 -
 arch/arm/configs/iop32x_defconfig           |   1 -
 arch/arm/configs/keystone_defconfig         |   1 -
 arch/arm/configs/lart_defconfig             |   1 -
 arch/arm/configs/netwinder_defconfig        |   1 -
 arch/arm/configs/versatile_defconfig        |   1 -
 arch/arm/configs/viper_defconfig            |   1 -
 arch/arm/configs/zeus_defconfig             |   1 -
 arch/ia64/configs/zx1_defconfig             |   1 -
 arch/m68k/configs/amiga_defconfig           |   1 -
 arch/m68k/configs/apollo_defconfig          |   1 -
 arch/m68k/configs/atari_defconfig           |   1 -
 arch/m68k/configs/bvme6000_defconfig        |   1 -
 arch/m68k/configs/hp300_defconfig           |   1 -
 arch/m68k/configs/mac_defconfig             |   1 -
 arch/m68k/configs/multi_defconfig           |   1 -
 arch/m68k/configs/mvme147_defconfig         |   1 -
 arch/m68k/configs/mvme16x_defconfig         |   1 -
 arch/m68k/configs/q40_defconfig             |   1 -
 arch/m68k/configs/sun3_defconfig            |   1 -
 arch/m68k/configs/sun3x_defconfig           |   1 -
 arch/mips/configs/cobalt_defconfig          |   1 -
 arch/mips/configs/decstation_64_defconfig   |   1 -
 arch/mips/configs/decstation_defconfig      |   1 -
 arch/mips/configs/decstation_r4k_defconfig  |   1 -
 arch/mips/configs/ip22_defconfig            |   1 -
 arch/mips/configs/ip32_defconfig            |   1 -
 arch/mips/configs/jazz_defconfig            |   1 -
 arch/mips/configs/malta_defconfig           |   1 -
 arch/mips/configs/malta_kvm_defconfig       |   1 -
 arch/mips/configs/maltaup_xpa_defconfig     |   1 -
 arch/mips/configs/rm200_defconfig           |   1 -
 arch/mips/configs/tb0219_defconfig          |   1 -
 arch/mips/configs/tb0226_defconfig          |   1 -
 arch/mips/configs/tb0287_defconfig          |   1 -
 arch/mips/configs/workpad_defconfig         |   1 -
 arch/parisc/configs/generic-32bit_defconfig |   1 -
 arch/powerpc/configs/linkstation_defconfig  |   1 -
 arch/powerpc/configs/mvme5100_defconfig     |   1 -
 arch/sh/configs/ap325rxa_defconfig          |   1 -
 arch/sh/configs/ecovec24_defconfig          |   1 -
 arch/sh/configs/landisk_defconfig           |   1 -
 arch/sh/configs/sdk7780_defconfig           |   1 -
 arch/sh/configs/se7724_defconfig            |   1 -
 arch/sh/configs/sh03_defconfig              |   1 -
 arch/sh/configs/sh7785lcr_32bit_defconfig   |   1 -
 arch/sh/configs/titan_defconfig             |   1 -
 fs/Kconfig                                  |   2 +-
 fs/lockd/svc.c                              |  24 +++------
 fs/nfs/callback.c                           |  66 ++++++++-----------------
 fs/nfs/nfs4state.c                          |   1 -
 fs/nfsd/Kconfig                             |  12 +----
 fs/nfsd/Makefile                            |   3 +-
 fs/nfsd/filecache.c                         |   6 +--
 fs/nfsd/flexfilelayout.c                    |   2 +-
 fs/nfsd/nfs4layouts.c                       |   2 +-
 fs/nfsd/nfs4state.c                         |  20 +++++---
 fs/nfsd/nfs4xdr.c                           |  10 ++++
 fs/nfsd/nfscache.c                          |  33 ++++++-------
 fs/nfsd/nfsctl.c                            |  10 ++--
 fs/nfsd/nfsd.h                              |   2 +-
 fs/nfsd/nfsfh.c                             |   4 --
 fs/nfsd/nfsfh.h                             |  20 --------
 fs/nfsd/nfsproc.c                           |   2 +-
 fs/nfsd/nfssvc.c                            |  25 +++-------
 fs/nfsd/trace.h                             | 107 ++++++++++++++++++----------------------
 fs/nfsd/vfs.c                               |   9 ----
 fs/nfsd/vfs.h                               |   2 -
 fs/nfsd/xdr.h                               |   2 +-
 include/linux/sunrpc/svc.h                  |  26 ++--------
 include/linux/sunrpc/svc_xprt.h             |  12 +++--
 include/linux/sunrpc/xdr.h                  |   2 +
 include/trace/bpf_probe.h                   |   6 +++
 include/trace/events/sunrpc.h               | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------
 include/trace/perf.h                        |   6 +++
 include/trace/trace_events.h                |  55 ++++++++++++++++++++-
 kernel/module.c                             |   2 +-
 kernel/trace/trace_events.c                 |   6 +++
 net/sunrpc/svc.c                            |  50 ++++++++++---------
 net/sunrpc/svc_xprt.c                       |  68 ++++++++++++++++----------
 net/sunrpc/svcauth.c                        |   2 +
 net/sunrpc/svcauth_unix.c                   |  60 +++++++++++++++++++++++
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c  |   2 +-
 88 files changed, 512 insertions(+), 450 deletions(-)

--
Chuck Lever

Re: [GIT PULL] nfsd changes for 5.18

[pr-tracker-bot]

The pull request you sent on Mon, 21 Mar 2022 14:12:31 +0000:

> git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git tags/nfsd-5.18

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/14705fda8f6273501930dfe1d679ad4bec209f52

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] nfsd changes for 5.18

[Igor Mammedov]

On Mon, 21 Mar 2022 14:12:31 +0000
Chuck Lever III <chuck.lever@oracle.com> wrote:

couldn't find offender patch on ML so replying here

> Hi Linus-
> 
> The following changes since commit 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3:
> 
>   Linux 5.17-rc6 (2022-02-27 14:36:33 -0800)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git tags/nfsd-5.18
> 
> for you to fetch changes up to 4fc5f5346592cdc91689455d83885b0af65d71b8:
> 
>   nfsd: fix using the correct variable for sizeof() (2022-03-20 12:49:38 -0400)
> 
> ----------------------------------------------------------------
> New features:
> - NFSv3 support in NFSD is now always built
> - Added NFSD support for the NFSv4 birth-time file attribute
[...]

> Ondrej Valousek (1):
>       nfsd: Add support for the birth time attribute

This patch regressed clients that support TIME_CREATE attribute.
Starting with this patch client might think that server supports
TIME_CREATE and start sending this attribute in its requests.
However kernel on server side (since this patch and to
current master) upon getting such request will return EINVAL.
(my guess is that TIME_CREATE not being decoded properly and
that messes up request parsing).

End result is unusable mount (unless it's treated as readonly).

Reproduces with current master (HEAD at e5524c2a1fc40) and MacOS
client (Big Sur or newest Monterey).

server is typical setup exporting files from XFS (Fedora36)

 #  rpcdebug -m nfsd -s all

on client:

 % mount -t nfs -o vers=4,rw,nfc,sec=sys testnas:/mnt  ~/test
 % touch ~/test/fff
     touch: test/fff: Invalid argument

server logs:

 nfsd: fh_compose(exp fd:00/128 fff, ino=0)
 NFSD: nfsd4_open filename  op_openowner 0000000000000000

Here is a request the touch generates:
        Network File System, Ops(6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
            [Program Version: 4]
            [V4 Procedure: COMPOUND (1)]
            Tag: create
            minorversion: 0
            Operations (count: 6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
                Opcode: PUTFH (22)
                Opcode: SAVEFH (32)
                Opcode: OPEN (18)
                    seqid: 0x00000004
                    share_access: OPEN4_SHARE_ACCESS_BOTH (3)
                    share_deny: OPEN4_SHARE_DENY_NONE (0)
                    clientid: 0xba93c9620aec46ea
                    owner: <DATA>
                    Open Type: OPEN4_CREATE (1)
                        Create Mode: UNCHECKED4 (0)
                        Attr mask: 0x00040002 (Mode, Time_Create)
                            reco_attr: Mode (33)
                            reco_attr: Time_Create (50)
                    Claim Type: CLAIM_NULL (0)
                        Name: fff

        [...]

when trying to copy file via GUI (Finder) it goes a different route
but ends up with error anyway and with leftover 0-length file on server
with messed up permissions, i.e.

open/create without Time_Create succeeds but followup
setattr with Time_Create fails EINVAL.

        Network File System, Ops(3): PUTFH, SETATTR, GETATTR
            [Program Version: 4]
            [V4 Procedure: COMPOUND (1)]
            Tag: setattr
            minorversion: 0
            Operations (count: 3): PUTFH, SETATTR, GETATTR
                Opcode: PUTFH (22)
                Opcode: SETATTR (34)
                    StateID
                    Attr mask: 0x00450002 (Mode, Time_Access_Set, Time_Create, Time_Modify_Set)
                        reco_attr: Mode (33)
                        reco_attr: Time_Access_Set (48)
                        reco_attr: Time_Create (50)
                        reco_attr: Time_Modify_Set (54)
                Opcode: GETATTR (9)
            [Main Opcode: SETATTR (34)]

[...]
> --
> Chuck Lever
> 
> 
> 

Re: [GIT PULL] nfsd changes for 5.18

[Chuck Lever III]

> On Jul 10, 2022, at 6:43 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> On Mon, 21 Mar 2022 14:12:31 +0000
> Chuck Lever III <chuck.lever@oracle.com> wrote:
> 
> couldn't find offender patch on ML so replying here

Probably:

https://lore.kernel.org/linux-nfs/AEC24099-5BC9-49C8-B759-920824F23F3C@oracle.com/


>> Hi Linus-
>> 
>> The following changes since commit 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3:
>> 
>> Linux 5.17-rc6 (2022-02-27 14:36:33 -0800)
>> 
>> are available in the Git repository at:
>> 
>> git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git tags/nfsd-5.18
>> 
>> for you to fetch changes up to 4fc5f5346592cdc91689455d83885b0af65d71b8:
>> 
>> nfsd: fix using the correct variable for sizeof() (2022-03-20 12:49:38 -0400)
>> 
>> ----------------------------------------------------------------
>> New features:
>> - NFSv3 support in NFSD is now always built
>> - Added NFSD support for the NFSv4 birth-time file attribute
> [...]
> 
>> Ondrej Valousek (1):
>> nfsd: Add support for the birth time attribute

Thank you for the report, Igor.


> This patch regressed clients that support TIME_CREATE attribute.
> Starting with this patch client might think that server supports
> TIME_CREATE and start sending this attribute in its requests.

Indeed, e377a3e698fb ("nfsd: Add support for the birth time
attribute") does not include a change to nfsd4_decode_fattr4()
that decodes the birth time attribute.

I don't immediately see another storage protocol stack in our
kernel that supports a client setting the birth time, so NFSD
might have to ignore the client-provided value.


> However kernel on server side (since this patch and to
> current master) upon getting such request will return EINVAL.
> (my guess is that TIME_CREATE not being decoded properly and
> that messes up request parsing).

I'll send a quick-and-dirty fix your way as we explore the
question of whether NFSD needs to ignore the birth time value
in this case.


> End result is unusable mount (unless it's treated as readonly).

That seems odd, and not clear whether that's a client or server
problem. I hope that will clear up once the server deals with
the time_create attribute appropriately.


> Reproduces with current master (HEAD at e5524c2a1fc40) and MacOS
> client (Big Sur or newest Monterey).
> 
> server is typical setup exporting files from XFS (Fedora36)
> 
> #  rpcdebug -m nfsd -s all
> 
> on client:
> 
> % mount -t nfs -o vers=4,rw,nfc,sec=sys testnas:/mnt  ~/test
> % touch ~/test/fff
>     touch: test/fff: Invalid argument
> 
> server logs:
> 
> nfsd: fh_compose(exp fd:00/128 fff, ino=0)
> NFSD: nfsd4_open filename  op_openowner 0000000000000000
> 
> Here is a request the touch generates:
>        Network File System, Ops(6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
>            [Program Version: 4]
>            [V4 Procedure: COMPOUND (1)]
>            Tag: create
>            minorversion: 0
>            Operations (count: 6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
>                Opcode: PUTFH (22)
>                Opcode: SAVEFH (32)
>                Opcode: OPEN (18)
>                    seqid: 0x00000004
>                    share_access: OPEN4_SHARE_ACCESS_BOTH (3)
>                    share_deny: OPEN4_SHARE_DENY_NONE (0)
>                    clientid: 0xba93c9620aec46ea
>                    owner: <DATA>
>                    Open Type: OPEN4_CREATE (1)
>                        Create Mode: UNCHECKED4 (0)
>                        Attr mask: 0x00040002 (Mode, Time_Create)
>                            reco_attr: Mode (33)
>                            reco_attr: Time_Create (50)
>                    Claim Type: CLAIM_NULL (0)
>                        Name: fff
> 
>        [...]
> 
> when trying to copy file via GUI (Finder) it goes a different route
> but ends up with error anyway and with leftover 0-length file on server
> with messed up permissions, i.e.

The current NFSv4 OPEN(CREATE) code path is still not right. Fixing
the TIME_CREATE problem should make this symptom go away for now,
but eventually that path will need to be restructured so that it
cannot leave a turd if the whole create process was not able to
complete.


> open/create without Time_Create succeeds but followup
> setattr with Time_Create fails EINVAL.
> 
>        Network File System, Ops(3): PUTFH, SETATTR, GETATTR
>            [Program Version: 4]
>            [V4 Procedure: COMPOUND (1)]
>            Tag: setattr
>            minorversion: 0
>            Operations (count: 3): PUTFH, SETATTR, GETATTR
>                Opcode: PUTFH (22)
>                Opcode: SETATTR (34)
>                    StateID
>                    Attr mask: 0x00450002 (Mode, Time_Access_Set, Time_Create, Time_Modify_Set)
>                        reco_attr: Mode (33)
>                        reco_attr: Time_Access_Set (48)
>                        reco_attr: Time_Create (50)
>                        reco_attr: Time_Modify_Set (54)
>                Opcode: GETATTR (9)
>            [Main Opcode: SETATTR (34)]
> 
> [...]
>> --
>> Chuck Lever

--
Chuck Lever

Re: [GIT PULL] nfsd changes for 5.18

[Jeff Layton]

On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> 
> > On Jul 10, 2022, at 6:43 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > On Mon, 21 Mar 2022 14:12:31 +0000
> > Chuck Lever III <chuck.lever@oracle.com> wrote:
> > 
> > couldn't find offender patch on ML so replying here
> 
> Probably:
> 
> https://lore.kernel.org/linux-nfs/AEC24099-5BC9-49C8-B759-920824F23F3C@oracle.com/
> 
> 
> > > Hi Linus-
> > > 
> > > The following changes since commit 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3:
> > > 
> > > Linux 5.17-rc6 (2022-02-27 14:36:33 -0800)
> > > 
> > > are available in the Git repository at:
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git tags/nfsd-5.18
> > > 
> > > for you to fetch changes up to 4fc5f5346592cdc91689455d83885b0af65d71b8:
> > > 
> > > nfsd: fix using the correct variable for sizeof() (2022-03-20 12:49:38 -0400)
> > > 
> > > ----------------------------------------------------------------
> > > New features:
> > > - NFSv3 support in NFSD is now always built
> > > - Added NFSD support for the NFSv4 birth-time file attribute
> > [...]
> > 
> > > Ondrej Valousek (1):
> > > nfsd: Add support for the birth time attribute
> 
> Thank you for the report, Igor.
> 
> 
> > This patch regressed clients that support TIME_CREATE attribute.
> > Starting with this patch client might think that server supports
> > TIME_CREATE and start sending this attribute in its requests.
> 
> Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> attribute") does not include a change to nfsd4_decode_fattr4()
> that decodes the birth time attribute.
> 
> I don't immediately see another storage protocol stack in our
> kernel that supports a client setting the birth time, so NFSD
> might have to ignore the client-provided value.
> 

Cephfs allows this. My thinking at the time that I implemented it was
that it should be settable for backup purposes, but this was possibly a
mistake. On most filesystems, the btime seems to be equivalent to inode
creation time and is read-only.

> 
> > However kernel on server side (since this patch and to
> > current master) upon getting such request will return EINVAL.
> > (my guess is that TIME_CREATE not being decoded properly and
> > that messes up request parsing).
> 
> I'll send a quick-and-dirty fix your way as we explore the
> question of whether NFSD needs to ignore the birth time value
> in this case.
> 
> 
> > End result is unusable mount (unless it's treated as readonly).
> 
> That seems odd, and not clear whether that's a client or server
> problem. I hope that will clear up once the server deals with
> the time_create attribute appropriately.
> 
> 
> > Reproduces with current master (HEAD at e5524c2a1fc40) and MacOS
> > client (Big Sur or newest Monterey).
> > 
> > server is typical setup exporting files from XFS (Fedora36)
> > 
> > #  rpcdebug -m nfsd -s all
> > 
> > on client:
> > 
> > % mount -t nfs -o vers=4,rw,nfc,sec=sys testnas:/mnt  ~/test
> > % touch ~/test/fff
> >     touch: test/fff: Invalid argument
> > 
> > server logs:
> > 
> > nfsd: fh_compose(exp fd:00/128 fff, ino=0)
> > NFSD: nfsd4_open filename  op_openowner 0000000000000000
> > 
> > Here is a request the touch generates:
> >        Network File System, Ops(6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
> >            [Program Version: 4]
> >            [V4 Procedure: COMPOUND (1)]
> >            Tag: create
> >            minorversion: 0
> >            Operations (count: 6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
> >                Opcode: PUTFH (22)
> >                Opcode: SAVEFH (32)
> >                Opcode: OPEN (18)
> >                    seqid: 0x00000004
> >                    share_access: OPEN4_SHARE_ACCESS_BOTH (3)
> >                    share_deny: OPEN4_SHARE_DENY_NONE (0)
> >                    clientid: 0xba93c9620aec46ea
> >                    owner: <DATA>
> >                    Open Type: OPEN4_CREATE (1)
> >                        Create Mode: UNCHECKED4 (0)
> >                        Attr mask: 0x00040002 (Mode, Time_Create)
> >                            reco_attr: Mode (33)
> >                            reco_attr: Time_Create (50)
> >                    Claim Type: CLAIM_NULL (0)
> >                        Name: fff
> > 
> >        [...]
> > 
> > when trying to copy file via GUI (Finder) it goes a different route
> > but ends up with error anyway and with leftover 0-length file on server
> > with messed up permissions, i.e.
> 
> The current NFSv4 OPEN(CREATE) code path is still not right. Fixing
> the TIME_CREATE problem should make this symptom go away for now,
> but eventually that path will need to be restructured so that it
> cannot leave a turd if the whole create process was not able to
> complete.
> 
> 
> > open/create without Time_Create succeeds but followup
> > setattr with Time_Create fails EINVAL.
> > 
> >        Network File System, Ops(3): PUTFH, SETATTR, GETATTR
> >            [Program Version: 4]
> >            [V4 Procedure: COMPOUND (1)]
> >            Tag: setattr
> >            minorversion: 0
> >            Operations (count: 3): PUTFH, SETATTR, GETATTR
> >                Opcode: PUTFH (22)
> >                Opcode: SETATTR (34)
> >                    StateID
> >                    Attr mask: 0x00450002 (Mode, Time_Access_Set, Time_Create, Time_Modify_Set)
> >                        reco_attr: Mode (33)
> >                        reco_attr: Time_Access_Set (48)
> >                        reco_attr: Time_Create (50)
> >                        reco_attr: Time_Modify_Set (54)
> >                Opcode: GETATTR (9)
> >            [Main Opcode: SETATTR (34)]
> > 
> > [...]
> > > --
> > > Chuck Lever
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Jeff Layton <jlayton@redhat.com>

Re: [GIT PULL] nfsd changes for 5.18

[Bruce Fields]

On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> > > This patch regressed clients that support TIME_CREATE attribute.
> > > Starting with this patch client might think that server supports
> > > TIME_CREATE and start sending this attribute in its requests.
> > 
> > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > attribute") does not include a change to nfsd4_decode_fattr4()
> > that decodes the birth time attribute.
> > 
> > I don't immediately see another storage protocol stack in our
> > kernel that supports a client setting the birth time, so NFSD
> > might have to ignore the client-provided value.
> > 
> 
> Cephfs allows this. My thinking at the time that I implemented it was
> that it should be settable for backup purposes, but this was possibly a
> mistake. On most filesystems, the btime seems to be equivalent to inode
> creation time and is read-only.

So supporting it as read-only seems reasonable.

Clearly, failing to decode the setattr attempt isn't the right way to do
that.  I'm not sure what exactly it should be doing--some kind of
permission error on any setattr containing TIME_CREATE?

--b.

Re: [GIT PULL] nfsd changes for 5.18

[Chuck Lever III]

> On Jul 11, 2022, at 2:19 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
>> On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
>>>> This patch regressed clients that support TIME_CREATE attribute.
>>>> Starting with this patch client might think that server supports
>>>> TIME_CREATE and start sending this attribute in its requests.
>>> 
>>> Indeed, e377a3e698fb ("nfsd: Add support for the birth time
>>> attribute") does not include a change to nfsd4_decode_fattr4()
>>> that decodes the birth time attribute.
>>> 
>>> I don't immediately see another storage protocol stack in our
>>> kernel that supports a client setting the birth time, so NFSD
>>> might have to ignore the client-provided value.
>>> 
>> 
>> Cephfs allows this. My thinking at the time that I implemented it was
>> that it should be settable for backup purposes, but this was possibly a
>> mistake. On most filesystems, the btime seems to be equivalent to inode
>> creation time and is read-only.
> 
> So supporting it as read-only seems reasonable.
> 
> Clearly, failing to decode the setattr attempt isn't the right way to do
> that.  I'm not sure what exactly it should be doing--some kind of
> permission error on any setattr containing TIME_CREATE?

I don't think that will work.

NFSD now asserts FATTR4_WORD1_TIME_CREATE when clients ask for
the mask of attributes it supports. That means the server has
to process GETATTR and SETATTR (and OPEN) operations that
contain FATTR4_WORD1_TIME_CREATE as not an error. The protocol
allows the server to indicate it ignored the time_create value
by clearing the FATTR4_WORD1_TIME_CREATE bit in the attribute
bitmask it returns in the reply.


--
Chuck Lever

Re: [GIT PULL] nfsd changes for 5.18

[Bruce Fields]

On Mon, Jul 11, 2022 at 06:24:01PM +0000, Chuck Lever III wrote:
> 
> 
> > On Jul 11, 2022, at 2:19 PM, Bruce Fields <bfields@fieldses.org> wrote:
> > 
> > On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> >> On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> >>>> This patch regressed clients that support TIME_CREATE attribute.
> >>>> Starting with this patch client might think that server supports
> >>>> TIME_CREATE and start sending this attribute in its requests.
> >>> 
> >>> Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> >>> attribute") does not include a change to nfsd4_decode_fattr4()
> >>> that decodes the birth time attribute.
> >>> 
> >>> I don't immediately see another storage protocol stack in our
> >>> kernel that supports a client setting the birth time, so NFSD
> >>> might have to ignore the client-provided value.
> >>> 
> >> 
> >> Cephfs allows this. My thinking at the time that I implemented it was
> >> that it should be settable for backup purposes, but this was possibly a
> >> mistake. On most filesystems, the btime seems to be equivalent to inode
> >> creation time and is read-only.
> > 
> > So supporting it as read-only seems reasonable.
> > 
> > Clearly, failing to decode the setattr attempt isn't the right way to do
> > that.  I'm not sure what exactly it should be doing--some kind of
> > permission error on any setattr containing TIME_CREATE?
> 
> I don't think that will work.
> 
> NFSD now asserts FATTR4_WORD1_TIME_CREATE when clients ask for
> the mask of attributes it supports. That means the server has
> to process GETATTR and SETATTR (and OPEN) operations that
> contain FATTR4_WORD1_TIME_CREATE as not an error.

Well, permissions or bad attribute values or other stuff may prevent
setting one of the attributes.

And setattr isn't guaranteed to be atomic, so I don't think you can
eliminate the possibility that part of it might succeed and part might
not.

But it might be more helpful to fail the whole thing up front if you
know part of it's going to fail?

> The protocol
> allows the server to indicate it ignored the time_create value
> by clearing the FATTR4_WORD1_TIME_CREATE bit in the attribute
> bitmask it returns in the reply.

Yes, I think you also return an error in that case, though.

--b.

Re: [GIT PULL] nfsd changes for 5.18

[Jeff Layton]

On Mon, 2022-07-11 at 14:36 -0400, Bruce Fields wrote:
> On Mon, Jul 11, 2022 at 06:24:01PM +0000, Chuck Lever III wrote:
> > 
> > 
> > > On Jul 11, 2022, at 2:19 PM, Bruce Fields <bfields@fieldses.org> wrote:
> > > 
> > > On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> > > > On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> > > > > > This patch regressed clients that support TIME_CREATE attribute.
> > > > > > Starting with this patch client might think that server supports
> > > > > > TIME_CREATE and start sending this attribute in its requests.
> > > > > 
> > > > > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > > > > attribute") does not include a change to nfsd4_decode_fattr4()
> > > > > that decodes the birth time attribute.
> > > > > 
> > > > > I don't immediately see another storage protocol stack in our
> > > > > kernel that supports a client setting the birth time, so NFSD
> > > > > might have to ignore the client-provided value.
> > > > > 
> > > > 
> > > > Cephfs allows this. My thinking at the time that I implemented it was
> > > > that it should be settable for backup purposes, but this was possibly a
> > > > mistake. On most filesystems, the btime seems to be equivalent to inode
> > > > creation time and is read-only.
> > > 
> > > So supporting it as read-only seems reasonable.
> > > 
> > > Clearly, failing to decode the setattr attempt isn't the right way to do
> > > that.  I'm not sure what exactly it should be doing--some kind of
> > > permission error on any setattr containing TIME_CREATE?
> > 
> > I don't think that will work.
> > 
> > NFSD now asserts FATTR4_WORD1_TIME_CREATE when clients ask for
> > the mask of attributes it supports. That means the server has
> > to process GETATTR and SETATTR (and OPEN) operations that
> > contain FATTR4_WORD1_TIME_CREATE as not an error.
> 
> Well, permissions or bad attribute values or other stuff may prevent
> setting one of the attributes.
> 
> And setattr isn't guaranteed to be atomic, so I don't think you can
> eliminate the possibility that part of it might succeed and part might
> not.
> 
> But it might be more helpful to fail the whole thing up front if you
> know part of it's going to fail?
> 

RFC5661 says:

   On either success or failure of the operation, the server will return
   the attrsset bitmask to represent what (if any) attributes were
   successfully set.  The attrsset in the response is a subset of the
   attrmask field of the obj_attributes field in the argument.

...and then later:

   A mask of the attributes actually set is returned by SETATTR in all
   cases.  That mask MUST NOT include attribute bits not requested to be
   set by the client.  If the attribute masks in the request and reply
   are equal, the status field in the reply MUST be NFS4_OK.

So, I think just clearing the bit and returning NFS4_OK should be fine.

If the mask ends up being 0 after clearing the bit though, it might be
reasonable to return something like NFS4ERR_ATTRNOTSUPP. That would be a
bit weird though since we do support it for GETATTR, hence my suggestion
for a NFS4ERR_ATTR_RO.

> > The protocol
> > allows the server to indicate it ignored the time_create value
> > by clearing the FATTR4_WORD1_TIME_CREATE bit in the attribute
> > bitmask it returns in the reply.
> 
> Yes, I think you also return an error in that case, though.
> 
> --b.
> 

-- 
Jeff Layton <jlayton@redhat.com>

Re: [GIT PULL] nfsd changes for 5.18

[Igor Mammedov]

On Mon, 11 Jul 2022 14:19:41 -0400
Bruce Fields <bfields@fieldses.org> wrote:

> On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> > On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:  
> > > > This patch regressed clients that support TIME_CREATE attribute.
> > > > Starting with this patch client might think that server supports
> > > > TIME_CREATE and start sending this attribute in its requests.  
> > > 
> > > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > > attribute") does not include a change to nfsd4_decode_fattr4()
> > > that decodes the birth time attribute.
> > > 
> > > I don't immediately see another storage protocol stack in our
> > > kernel that supports a client setting the birth time, so NFSD
> > > might have to ignore the client-provided value.
> > >   
> > 
> > Cephfs allows this. My thinking at the time that I implemented it was
> > that it should be settable for backup purposes, but this was possibly a
> > mistake. On most filesystems, the btime seems to be equivalent to inode
> > creation time and is read-only.  
> 
> So supporting it as read-only seems reasonable.
> 
> Clearly, failing to decode the setattr attempt isn't the right way to do
> that.  I'm not sure what exactly it should be doing--some kind of
> permission error on any setattr containing TIME_CREATE?

erroring out on TIME_CREATE will break client that try to
set this attribute (legitimately). That's what by chance 
happening with current master (return error when TIME_CREATE
is present).

As long as server advertises support for TIME_CREATE
it should not error out when client sends it if spec permits
such use.

I think ignoring this attribute like Chuck has proposed
is acceptable (if one ignores archiving use case where
setting it makes sense).

Alternatively if folks inclined towards erroring out,
there should be a way to optout or optin from TIME_CREATE support,
to keep existing clients working + a sane error message so users
won't have to debug kernel to figure out what's wrong with
their setup.

> --b.
> 

Re: [GIT PULL] nfsd changes for 5.18

[Bruce Fields]

On Tue, Jul 12, 2022 at 10:27:46AM +0200, Igor Mammedov wrote:
> On Mon, 11 Jul 2022 14:19:41 -0400
> Bruce Fields <bfields@fieldses.org> wrote:
> 
> > On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> > > On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:  
> > > > > This patch regressed clients that support TIME_CREATE attribute.
> > > > > Starting with this patch client might think that server supports
> > > > > TIME_CREATE and start sending this attribute in its requests.  
> > > > 
> > > > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > > > attribute") does not include a change to nfsd4_decode_fattr4()
> > > > that decodes the birth time attribute.
> > > > 
> > > > I don't immediately see another storage protocol stack in our
> > > > kernel that supports a client setting the birth time, so NFSD
> > > > might have to ignore the client-provided value.
> > > >   
> > > 
> > > Cephfs allows this. My thinking at the time that I implemented it was
> > > that it should be settable for backup purposes, but this was possibly a
> > > mistake. On most filesystems, the btime seems to be equivalent to inode
> > > creation time and is read-only.  
> > 
> > So supporting it as read-only seems reasonable.
> > 
> > Clearly, failing to decode the setattr attempt isn't the right way to do
> > that.  I'm not sure what exactly it should be doing--some kind of
> > permission error on any setattr containing TIME_CREATE?
> 
> erroring out on TIME_CREATE will break client that try to
> set this attribute (legitimately). That's what by chance 
> happening with current master (return error when TIME_CREATE
> is present).

Hang on, now--our current server completely fails to decode any RPC
including a SETATTR that attempts to set TIME_CREATE, which means it
isn't able to return a useful error or tell the client which attribute
was the problem.

It's not too surprising that that would cause a problem for a client.

But failures to set supported attributes are completely normal, and if
mounts are failing completely because of that, something is really very
wrong with the client.

Could you first retest with a server that's patched to at least decode
the attribute correctly?  I suspect that may be enough.  If not, then
the client in question has a more interesting problem on its hands.

--b.

Re: [GIT PULL] nfsd changes for 5.18

[Bruce Fields]

On Mon, Jul 11, 2022 at 02:56:40PM -0400, Jeff Layton wrote:
> On Mon, 2022-07-11 at 14:36 -0400, Bruce Fields wrote:
> > On Mon, Jul 11, 2022 at 06:24:01PM +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Jul 11, 2022, at 2:19 PM, Bruce Fields <bfields@fieldses.org> wrote:
> > > > 
> > > > On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> > > > > On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> > > > > > > This patch regressed clients that support TIME_CREATE attribute.
> > > > > > > Starting with this patch client might think that server supports
> > > > > > > TIME_CREATE and start sending this attribute in its requests.
> > > > > > 
> > > > > > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > > > > > attribute") does not include a change to nfsd4_decode_fattr4()
> > > > > > that decodes the birth time attribute.
> > > > > > 
> > > > > > I don't immediately see another storage protocol stack in our
> > > > > > kernel that supports a client setting the birth time, so NFSD
> > > > > > might have to ignore the client-provided value.
> > > > > > 
> > > > > 
> > > > > Cephfs allows this. My thinking at the time that I implemented it was
> > > > > that it should be settable for backup purposes, but this was possibly a
> > > > > mistake. On most filesystems, the btime seems to be equivalent to inode
> > > > > creation time and is read-only.
> > > > 
> > > > So supporting it as read-only seems reasonable.
> > > > 
> > > > Clearly, failing to decode the setattr attempt isn't the right way to do
> > > > that.  I'm not sure what exactly it should be doing--some kind of
> > > > permission error on any setattr containing TIME_CREATE?
> > > 
> > > I don't think that will work.
> > > 
> > > NFSD now asserts FATTR4_WORD1_TIME_CREATE when clients ask for
> > > the mask of attributes it supports. That means the server has
> > > to process GETATTR and SETATTR (and OPEN) operations that
> > > contain FATTR4_WORD1_TIME_CREATE as not an error.
> > 
> > Well, permissions or bad attribute values or other stuff may prevent
> > setting one of the attributes.
> > 
> > And setattr isn't guaranteed to be atomic, so I don't think you can
> > eliminate the possibility that part of it might succeed and part might
> > not.
> > 
> > But it might be more helpful to fail the whole thing up front if you
> > know part of it's going to fail?
> > 
> 
> RFC5661 says:
> 
>    On either success or failure of the operation, the server will return
>    the attrsset bitmask to represent what (if any) attributes were
>    successfully set.  The attrsset in the response is a subset of the
>    attrmask field of the obj_attributes field in the argument.
> 
> ...and then later:
> 
>    A mask of the attributes actually set is returned by SETATTR in all
>    cases.  That mask MUST NOT include attribute bits not requested to be
>    set by the client.  If the attribute masks in the request and reply
>    are equal, the status field in the reply MUST be NFS4_OK.

For some reason I thought the converse was true too (if the masks
differ, then the server should return an error).  But you're right, I
don't see that in the spec.

> So, I think just clearing the bit and returning NFS4_OK should be fine.

I suppose.

Nevertheless, the spec gives the option of returning both an error and a
bitmap, and to me it seems more helpful to take advantage of the
opportunity to tell the client both which attribute(s) failed and (to
the extent possible) why.  ??

> If the mask ends up being 0 after clearing the bit though, it might be
> reasonable to return something like NFS4ERR_ATTRNOTSUPP. That would be a
> bit weird though since we do support it for GETATTR, hence my suggestion
> for a NFS4ERR_ATTR_RO.

That might be useful.

--b.

Re: [GIT PULL] nfsd changes for 5.18

[Igor Mammedov]

On Tue, 12 Jul 2022 07:42:11 -0400
Bruce Fields <bfields@fieldses.org> wrote:

> On Tue, Jul 12, 2022 at 10:27:46AM +0200, Igor Mammedov wrote:
> > On Mon, 11 Jul 2022 14:19:41 -0400
> > Bruce Fields <bfields@fieldses.org> wrote:
> >   
> > > On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:  
> > > > On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:    
> > > > > > This patch regressed clients that support TIME_CREATE attribute.
> > > > > > Starting with this patch client might think that server supports
> > > > > > TIME_CREATE and start sending this attribute in its requests.    
> > > > > 
> > > > > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > > > > attribute") does not include a change to nfsd4_decode_fattr4()
> > > > > that decodes the birth time attribute.
> > > > > 
> > > > > I don't immediately see another storage protocol stack in our
> > > > > kernel that supports a client setting the birth time, so NFSD
> > > > > might have to ignore the client-provided value.
> > > > >     
> > > > 
> > > > Cephfs allows this. My thinking at the time that I implemented it was
> > > > that it should be settable for backup purposes, but this was possibly a
> > > > mistake. On most filesystems, the btime seems to be equivalent to inode
> > > > creation time and is read-only.    
> > > 
> > > So supporting it as read-only seems reasonable.
> > > 
> > > Clearly, failing to decode the setattr attempt isn't the right way to do
> > > that.  I'm not sure what exactly it should be doing--some kind of
> > > permission error on any setattr containing TIME_CREATE?  
> > 
> > erroring out on TIME_CREATE will break client that try to
> > set this attribute (legitimately). That's what by chance 
> > happening with current master (return error when TIME_CREATE
> > is present).  
> 
> Hang on, now--our current server completely fails to decode any RPC
> including a SETATTR that attempts to set TIME_CREATE, which means it
> isn't able to return a useful error or tell the client which attribute
> was the problem.
> 
> It's not too surprising that that would cause a problem for a client.
> 
> But failures to set supported attributes are completely normal, and if
> mounts are failing completely because of that, something is really very
> wrong with the client.

returning unsupported attribute error might work, but as Chuck mentioned
we do kind of support TIME_CREATE for some requests so client might be
confused when server itself sends this attribute while errors out when
client tries to send it.
What I'm saying if we are to try returning error in this case
it should be tested with variety of clients before committing
to this approach. (meanwhile decoding and ignoring attribute
with Chuck's patch fixes immediate issue).

> Could you first retest with a server that's patched to at least decode
> the attribute correctly?  I suspect that may be enough.  If not, then
it does work with fixed decoding path:
 (i.e. patched with https://lore.kernel.org/lkml/A4F0C111-B2EB-4325-AC6A-4A80BD19DA43@oracle.com/T/)

> the client in question has a more interesting problem on its hands.
> 
> --b.
>