sahlberg / libnfs

NFS client library
Other
532 stars 203 forks source link

Personal/linuxsmiths/add vectored read write task #485

Closed linuxsmiths closed 3 months ago

linuxsmiths commented 3 months ago

Added the following lowlevel APIs for vectored read and write.

rpc_nfs3_readv_task rpc_nfs3_writev_task rpc_nfs4_readv_task rpc_nfs4_writev_task

Done basic testing. More testing underway.

linuxsmiths commented 3 months ago

Still auditing and testing, will update once done.. don't merge yet

linuxsmiths commented 3 months ago

Still auditing and testing, will update once done.. don't merge yet

Completed some non-trivial testing, successfully. Pls go ahead with the review.

linuxsmiths commented 3 months ago

@sahlberg , I've done enough validation and I'm comfortable with the change now. Pls review and merge. Thanks.

linuxsmiths commented 3 months ago

Please do the suggested changes for allocate_pdu2/allocate_pdu3 as well as the missing checks for alloc failures.

Done.

linuxsmiths commented 3 months ago

@sahlberg, do you have more comments or concerns or anything else you are awaiting? I need to add more (unrelated) changes on top of this which I need for my product.

<Request> Currently I don't merge the changes to my (forked) repo's master till you merge it in your (original) repo as I'd want the repos to be in sync. I understand you are busy with your other work, but since we've our development going on at a rapid pace, I might have to unlink this process and merge to my repo and possibly create a PR for the sahlberg/libnfs repo. Would like to know your thoughts. </Request>

sahlberg commented 3 months ago

The changes look mostly good but they breaks sec=krb5p for large reads.

Do you have access to a kerberized nfs environment to test with?

sahlberg commented 3 months ago

I merged it and I will figure out what is wrong with krb5p and fix. Don't worry about it. Not sure if large reads have been tested fully with krb5p so the breakage might be unrelated to your pull request.

linuxsmiths commented 3 months ago

I merged it and I will figure out what is wrong with krb5p and fix. Don't worry about it. Not sure if large reads have been tested fully with krb5p so the breakage might be unrelated to your pull request.

Thanks @sahlberg. Sorry I don't have access to a kerberized setup currently. When you test it, does it fail only for vectored reads or even vectored writes? Also, do non-vectored reads or iovcnt=1 work fine? Also, is it the size of the read that matters or the number of vector elements? How about small reads (say 4096 bytes) but with multiple vectors? If only iovcnt>1 reads are broken, till they are fixed, shall we disallow vectored reads for krb5* with a comment explaining why?

sahlberg commented 3 months ago

I have started digging into it and it is unrelated to your changes. krb5p has not been used or tested much so the breakage is unrelated to your changes. It seems to be related to zero-zopy + krb5p. Possibly also when the server returns a short read.

Unrelated to your changes so don't worry about it. It is my bug and I will track it down and fix it.

linuxsmiths commented 3 months ago

Thanks, appreciate! Another thing that I do not test (and worry so much about) is server mode, so if you can pls test and make sure that's not broken.