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 <[email protected]>
---
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
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: [email protected] <[email protected]> on behalf of Tigran Mkrtchyan <[email protected]>
Sent: Monday, August 20, 2018 2:56:08 AM
To: [email protected]
Cc: [email protected]; [email protected]; 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 <[email protected]>
---
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
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: [email protected] <[email protected]> on behalf of Rick Macklem <[email protected]>
Sent: Monday, August 20, 2018 8:51:14 AM
To: Tigran Mkrtchyan; [email protected]
Cc: [email protected]; [email protected]
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: [email protected] <[email protected]> on behalf of Tigran Mkrtchyan <[email protected]>
Sent: Monday, August 20, 2018 2:56:08 AM
To: [email protected]
Cc: [email protected]; [email protected]; 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 <[email protected]>
---
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
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);
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?
Thanks,
Tigran.
----- Original Message -----
> From: "Rick Macklem" <[email protected]>
> To: "Tigran Mkrtchyan" <[email protected]>, "linux-nfs" <[email protected]>
> Cc: [email protected], "Anna Schumaker" <[email protected]>
> 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: [email protected] <[email protected]> on
> behalf of Rick Macklem <[email protected]>
> Sent: Monday, August 20, 2018 8:51:14 AM
> To: Tigran Mkrtchyan; [email protected]
> Cc: [email protected]; [email protected]
> 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: [email protected] <[email protected]> on
> behalf of Tigran Mkrtchyan <[email protected]>
> Sent: Monday, August 20, 2018 2:56:08 AM
> To: [email protected]
> Cc: [email protected]; [email protected]; 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 <[email protected]>
> ---
> 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
> On Aug 20, 2018, at 1:41 PM, Mkrtchyan, Tigran =
<[email protected]> wrote:
>=20
> Hi Rick,
>=20
> draft-19 says:
>=20
> 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.
>=20
> 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,
>=20
> + stateid =3D nfs4_ff_layout_select_ds_stateid(lseg, idx);
> + if (stateid)
> + nfs4_stateid_copy(&hdr->args.stateid, stateid);
>=20
>=20
> 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.
>=20
> Trond, Tom, can you comment on it?
Yes, I agree, the server always provides the stateid.
>=20
> Thanks,
> Tigran.
>=20
> ----- Original Message -----
>> From: "Rick Macklem" <[email protected]>
>> To: "Tigran Mkrtchyan" <[email protected]>, "linux-nfs" =
<[email protected]>
>> Cc: [email protected], "Anna Schumaker" =
<[email protected]>
>> Sent: Monday, August 20, 2018 3:18:00 PM
>> Subject: Re: [PATCH] nfs4: flex_file: ignore synthetic uid/gid for =
tightly coupled DSes
>=20
>> 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
>>=20
>> Thanks for doing this, rick
>>=20
>> ________________________________________
>> From: [email protected] =
<[email protected]> on
>> behalf of Rick Macklem <[email protected]>
>> Sent: Monday, August 20, 2018 8:51:14 AM
>> To: Tigran Mkrtchyan; [email protected]
>> Cc: [email protected]; [email protected]
>> Subject: Re: [PATCH] nfs4: flex_file: ignore synthetic uid/gid for =
tightly
>> coupled DSes
>>=20
>> 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).
>>=20
>> 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.
>>=20
>> http://people.freebsd.org/~rmacklem/flexfile.patch
>> (Sorry, I don't know git;-)
>>=20
>> Thanks for doing this and I'll post if my testing finds problems, =
rick
>>=20
>>=20
>> ________________________________________
>> From: [email protected] =
<[email protected]> on
>> behalf of Tigran Mkrtchyan <[email protected]>
>> Sent: Monday, August 20, 2018 2:56:08 AM
>> To: [email protected]
>> Cc: [email protected]; [email protected]; Tigran =
Mkrtchyan
>> Subject: [PATCH] nfs4: flex_file: ignore synthetic uid/gid for =
tightly coupled
>> DSes
>>=20
>> for tightly coupled DSes client must ignore provided synthetic uid =
and
>> gid as stated in draft-ietf-nfsv4-flex-files-19#section-5.1.
>>=20
>> Signed-off-by: Tigran Mkrtchyan <[email protected]>
>> ---
>> fs/nfs/flexfilelayout/flexfilelayoutdev.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>=20
>> 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 =3D FF_LAYOUT_COMP(lseg, =
ds_idx);
>> struct rpc_cred *cred;
>>=20
>> - if (mirror) {
>> + if (mirror && =
!mirror->mirror_ds->ds_versions[0].tightly_coupled) {
>> cred =3D ff_layout_get_mirror_cred(mirror, =
lseg->pls_range.iomode);
>> if (!cred)
>> cred =3D get_rpccred(mdscred);
>> --
>> 2.17.1
Tom Haynes wrote:
>> On Aug 20, 2018, at 1:41 PM, Mkrtchyan, Tigran <[email protected]> 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.)
Doing it unconditionally makes the patch even simpler.
rick
>
> Thanks,
> Tigran.
>
> ----- Original Message -----
>> From: "Rick Macklem" <[email protected]>
>> To: "Tigran Mkrtchyan" <[email protected]>, "linux-nfs" <[email protected]>
>> Cc: [email protected], "Anna Schumaker" <[email protected]>
>> 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: [email protected] <[email protected]> on
>> behalf of Rick Macklem <[email protected]>
>> Sent: Monday, August 20, 2018 8:51:14 AM
>> To: Tigran Mkrtchyan; [email protected]
>> Cc: [email protected]; [email protected]
>> 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: [email protected] <[email protected]> on
>> behalf of Tigran Mkrtchyan <[email protected]>
>> Sent: Monday, August 20, 2018 2:56:08 AM
>> To: [email protected]
>> Cc: [email protected]; [email protected]; 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 <[email protected]>
>> ---
>> 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
----- Original Message -----
> From: "Rick Macklem" <[email protected]>
> To: "Tom Haynes" <[email protected]>, "Tigran Mkrtchyan" <[email protected]>
> Cc: "linux-nfs" <[email protected]>, "trondmy" <[email protected]>, "Anna Schumaker"
> <[email protected]>
> 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 <[email protected]> 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" <[email protected]>
>>> To: "Tigran Mkrtchyan" <[email protected]>, "linux-nfs"
>>> <[email protected]>
>>> Cc: [email protected], "Anna Schumaker" <[email protected]>
>>> 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: [email protected] <[email protected]> on
>>> behalf of Rick Macklem <[email protected]>
>>> Sent: Monday, August 20, 2018 8:51:14 AM
>>> To: Tigran Mkrtchyan; [email protected]
>>> Cc: [email protected]; [email protected]
>>> 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: [email protected] <[email protected]> on
>>> behalf of Tigran Mkrtchyan <[email protected]>
>>> Sent: Monday, August 20, 2018 2:56:08 AM
>>> To: [email protected]
>>> Cc: [email protected]; [email protected]; 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 <[email protected]>
>>> ---
>>> 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
Mkrtchyan, Tigran wrote:
[stuff snipped for brevity]
>Rick Macklem wrote:
>> 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:
Oops, yes, you are correct. (It has been a while since I did this.)
The unpatched driver uses whatever it would use for the MDS (an open/lock/?
stated). If the server sends the same stateid in the layout, then you wouldn't notice.
(I'd guess Primary Data is using NFSv3 data servers, so they wouldn't notice either.)
Oh, and ignore my comment about taking out the code at the end of
ff_layout_read_prepare_v4() and ff_layout_write_prepare_v4() unless you know
of a better way to handle the failure case than filling in the stateid that would
be used against the MDS when it can't get the ffds_stateid.
rick
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" <[email protected]>
>>> To: "Tigran Mkrtchyan" <[email protected]>, "linux-nfs"
>>> <[email protected]>
>>> Cc: [email protected], "Anna Schumaker" <[email protected]>
>>> 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: [email protected] <[email protected]> on
>>> behalf of Rick Macklem <[email protected]>
>>> Sent: Monday, August 20, 2018 8:51:14 AM
>>> To: Tigran Mkrtchyan; [email protected]
>>> Cc: [email protected]; [email protected]
>>> 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: [email protected] <[email protected]> on
>>> behalf of Tigran Mkrtchyan <[email protected]>
>>> Sent: Monday, August 20, 2018 2:56:08 AM
>>> To: [email protected]
>>> Cc: [email protected]; [email protected]; 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 <[email protected]>
>>> ---
>>> 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
Mkrtchyan, Tigran wrote:
>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);
>
>
>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?
I've updated http://people.freebsd.org/~rmacklem/flexfilestateid.patch
to do the above for both loose and tightly coupled as you suggested.
When used with your little cred patch, everything seems ok against the
FreeBSD server (which is tightly coupled and uses ffds_stateids that are
different from the open/lock... ones).
rick
Thanks,
Tigran.
----- Original Message -----
> From: "Rick Macklem" <[email protected]>
> To: "Tigran Mkrtchyan" <[email protected]>, "linux-nfs" <[email protected]>
> Cc: [email protected], "Anna Schumaker" <[email protected]>
> 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: [email protected] <[email protected]> on
> behalf of Rick Macklem <[email protected]>
> Sent: Monday, August 20, 2018 8:51:14 AM
> To: Tigran Mkrtchyan; [email protected]
> Cc: [email protected]; [email protected]
> 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: [email protected] <[email protected]> on
> behalf of Tigran Mkrtchyan <[email protected]>
> Sent: Monday, August 20, 2018 2:56:08 AM
> To: [email protected]
> Cc: [email protected]; [email protected]; 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 <[email protected]>
> ---
> 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