----- Original Message -----
> From: "Rick Macklem" <rmacklem@xxxxxxxxxxx>
> To: "Tom Haynes" <loghyr@xxxxxxxxx>, "Tigran Mkrtchyan" <tigran.mkrtchyan@xxxxxxx>
> Cc: "linux-nfs" <linux-nfs@xxxxxxxxxxxxxxx>, "trondmy" <trondmy@xxxxxxxxxxxxxxx>, "Anna Schumaker"
> <Anna.Schumaker@xxxxxxxxxx>
> Sent: Monday, August 20, 2018 11:16:22 PM
> Subject: Re: [PATCH] nfs4: flex_file: ignore synthetic uid/gid for tightly coupled DSes
> Tom Haynes wrote:
>>> On Aug 20, 2018, at 1:41 PM, Mkrtchyan, Tigran <tigran.mkrtchyan@xxxxxxx> wrote:
>>>
>>> Hi Rick,
>>>
>>> draft-19 says:
>>>
>>> For tight coupling, ffds_stateid provides the stateid to be used by
>>> the client to access the file. For loose coupling and a NFSv4
>>> storage device, the client will have to use an anonymous stateid to
>>> perform I/O on the storage device. With no control protocol, the
>>> metadata server stateid can not be used to provide a global stateid
>>> model. Thus the server MUST set the ffds_stateid to be the anonymous
>>> stateid.
>>>
>>> To me, the last sentence sounds like a clear statement, that it's server
>>> responsibility to provide either global or anonymous stateid and client
>>> should just use it as is. IOW,
>>>
>>> + stateid = nfs4_ff_layout_select_ds_stateid(lseg, idx);
>>> + if (stateid)
>>> + nfs4_stateid_copy(&hdr->args.stateid, stateid);
>>>
> If you do this unconditionally, I think you can get rid of the code that looks
> like:
> if (nfs4_set_rw_stateid(&hdr->args.stateid, hdr->args.context,
> hdr->args.lock_context, FMODE_READ) == -EIO)
> rpc_exit(task, -EIO); /* lost lock, terminate I/O */
> at the end of ff_layout_read_prepare_v4() and similar but with FMODE_WRITE
> at the end of ff_layout_write_prepare_v4().
>
>>>
>>> must happen independent from ds being loose or tight coupled. As our DSes
>>> use the same stateid's as MDS, we did see that provided stateid is not used.
>>>
>>> Trond, Tom, can you comment on it?
>>
>>
>>Yes, I agree, the server always provides the stateid.
> Sure. Since the above states that the loosely coupled server must fill it in as
> the anonymous stated, the client will end up using the anonymous stated
> for loosely coupled, whether it uses ffds_stateid or just sets it to the
> anonymous stated. (Unpatched, the client always use the anonymous stated,
> including for tightly coupled.)
I am not sure that's true, as our DSes will reject such IO requests. And we
do use flex_files in production:
Frame 96: 264 bytes on wire (2112 bits), 264 bytes captured (2112 bits) on interface 0
Linux cooked capture
Internet Protocol Version 4, Src: 131.169.xx, Dst: 131.169.xx
Transmission Control Protocol, Src Port: 946, Dst Port: 32049, Seq: 925, Ack: 569, Len: 196
Remote Procedure Call, Type:Call XID:0xa09b9261
Network File System, Ops(3): SEQUENCE, PUTFH, READ
[Program Version: 4]
[V4 Procedure: COMPOUND (1)]
Tag: <EMPTY>
length: 0
contents: <EMPTY>
minorversion: 1
Operations (count: 3): SEQUENCE, PUTFH, READ
Opcode: SEQUENCE (53)
sessionid: 5b7b31b9000000060000000000000001
seqid: 0x00000002
slot id: 0
high slot id: 0
cache this?: No
Opcode: PUTFH (22)
FileHandle
length: 27
[hash (CRC-32): 0x4c4a3909]
FileHandle: 01caffee000000008d61f80c000d00000800000000002887...
Opcode: READ (25)
StateID
[StateID Hash: 0x71b8]
StateID seqid: 0
StateID Other: 5b7b31b60000000500000001
[StateID Other hash: 0xbecf]
offset: 0
count: 1015
[Main Opcode: READ (25)]
Tigran.
>
> Doing it unconditionally makes the patch even simpler.
>
> rick
>
>>
>> Thanks,
>> Tigran.
>>
>> ----- Original Message -----
>>> From: "Rick Macklem" <rmacklem@xxxxxxxxxxx>
>>> To: "Tigran Mkrtchyan" <tigran.mkrtchyan@xxxxxxx>, "linux-nfs"
>>> <linux-nfs@xxxxxxxxxxxxxxx>
>>> Cc: trondmy@xxxxxxxxxxxxxxx, "Anna Schumaker" <Anna.Schumaker@xxxxxxxxxx>
>>> Sent: Monday, August 20, 2018 3:18:00 PM
>>> Subject: Re: [PATCH] nfs4: flex_file: ignore synthetic uid/gid for tightly
>>> coupled DSes
>>
>>> I just put a patch for the stated part (stripped out my version of the cred
>>> stuff, which I noticed I missed commit in it;) so it might be easier to read.
>>> It is here:
>>> http://people.freebsd.org/~rmacklem/flexfilestateid.patch
>>>
>>> Thanks for doing this, rick
>>>
>>> ________________________________________
>>> From: linux-nfs-owner@xxxxxxxxxxxxxxx <linux-nfs-owner@xxxxxxxxxxxxxxx> on
>>> behalf of Rick Macklem <rmacklem@xxxxxxxxxxx>
>>> Sent: Monday, August 20, 2018 8:51:14 AM
>>> To: Tigran Mkrtchyan; linux-nfs@xxxxxxxxxxxxxxx
>>> Cc: trondmy@xxxxxxxxxxxxxxx; Anna.Schumaker@xxxxxxxxxx
>>> Subject: Re: [PATCH] nfs4: flex_file: ignore synthetic uid/gid for tightly
>>> coupled DSes
>>>
>>> Thanks. I'll test this later to-day. (I did a slightly more complex version
>>> of the fix outside of the ff_layout_get_ds_cred() call, but yours is
>>> definitely simpler).
>>>
>>> There is also the "stateid", which I believe is supposed to be the one in
>>> the layout for the tightly coupled case (for NFSv4 I/O ops to the DS).
>>> My patch is here and has the changes for stated as well as cred.
>>>
>>> http://people.freebsd.org/~rmacklem/flexfile.patch
>>> (Sorry, I don't know git;-)
>>>
>>> Thanks for doing this and I'll post if my testing finds problems, rick
>>>
>>>
>>> ________________________________________
>>> From: linux-nfs-owner@xxxxxxxxxxxxxxx <linux-nfs-owner@xxxxxxxxxxxxxxx> on
>>> behalf of Tigran Mkrtchyan <tigran.mkrtchyan@xxxxxxx>
>>> Sent: Monday, August 20, 2018 2:56:08 AM
>>> To: linux-nfs@xxxxxxxxxxxxxxx
>>> Cc: trondmy@xxxxxxxxxxxxxxx; Anna.Schumaker@xxxxxxxxxx; Tigran Mkrtchyan
>>> Subject: [PATCH] nfs4: flex_file: ignore synthetic uid/gid for tightly coupled
>>> DSes
>>>
>>> for tightly coupled DSes client must ignore provided synthetic uid and
>>> gid as stated in draft-ietf-nfsv4-flex-files-19#section-5.1.
>>>
>>> Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@xxxxxxx>
>>> ---
>>> fs/nfs/flexfilelayout/flexfilelayoutdev.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfs/flexfilelayout/flexfilelayoutdev.c
>>> b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
>>> index d62279d3fc5d..290625cfd369 100644
>>> --- a/fs/nfs/flexfilelayout/flexfilelayoutdev.c
>>> +++ b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
>>> @@ -452,7 +452,7 @@ ff_layout_get_ds_cred(struct pnfs_layout_segment *lseg, u32
>>> ds_idx,
>>> struct nfs4_ff_layout_mirror *mirror = FF_LAYOUT_COMP(lseg, ds_idx);
>>> struct rpc_cred *cred;
>>>
>>> - if (mirror) {
>>> + if (mirror && !mirror->mirror_ds->ds_versions[0].tightly_coupled) {
>>> cred = ff_layout_get_mirror_cred(mirror, lseg->pls_range.iomode);
>>> if (!cred)
>>> cred = get_rpccred(mdscred);
>>> --
> >> 2.17.1
