| Message ID | EF97E1F5-B70F-4F9F-AC6D-7B48336AE3E5@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] nfsd changes for 5.18 | expand |
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!
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 > > >
> 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
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 > > >
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.
> 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
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.
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. >
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. >
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.
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.
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. >
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