Re: [net-next][PATCH v2 1/2] rds: handle unsupported rdma request to fs dax memory

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, May 10, 2019 at 11:02:35AM -0700, santosh.shilimkar@oracle.com wrote:
> 
> 
> On 5/10/19 10:55 AM, Jason Gunthorpe wrote:
> > On Fri, May 10, 2019 at 09:11:24AM -0700, Santosh Shilimkar wrote:
> > > On 5/10/2019 5:54 AM, Jason Gunthorpe wrote:
> > > > On Mon, Apr 29, 2019 at 04:37:19PM -0700, Santosh Shilimkar wrote:
> > > > > From: Hans Westgaard Ry <hans.westgaard.ry@oracle.com>
> > > > > 
> > > > > RDS doesn't support RDMA on memory apertures that require On Demand
> > > > > Paging (ODP), such as FS DAX memory. User applications can try to use
> > > > > RDS to perform RDMA over such memories and since it doesn't report any
> > > > > failure, it can lead to unexpected issues like memory corruption when
> > > > > a couple of out of sync file system operations like ftruncate etc. are
> > > > > performed.
> > > > 
> > > > This comment doesn't make any sense..
> > > > 
> > > > > The patch adds a check so that such an attempt to RDMA to/from memory
> > > > > apertures requiring ODP will fail.
> > > > > 
> > > > > Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
> > > > > Reviewed-tested-by: Zhu Yanjun <yanjun.zhu@oracle.com>
> > > > > Signed-off-by: Hans Westgaard Ry <hans.westgaard.ry@oracle.com>
> > > > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
> > > > >    net/rds/rdma.c | 5 +++--
> > > > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/net/rds/rdma.c b/net/rds/rdma.c
> > > > > index 182ab84..e0a6b72 100644
> > > > > +++ b/net/rds/rdma.c
> > > > > @@ -158,8 +158,9 @@ static int rds_pin_pages(unsigned long user_addr, unsigned int nr_pages,
> > > > >    {
> > > > >    	int ret;
> > > > > -	ret = get_user_pages_fast(user_addr, nr_pages, write, pages);
> > > > > -
> > > > > +	/* get_user_pages return -EOPNOTSUPP for fs_dax memory */
> > > > > +	ret = get_user_pages_longterm(user_addr, nr_pages,
> > > > > +				      write, pages, NULL);
> > > > 
> > > > GUP is supposed to fully work on DAX filesystems.
> > > > 
> > > Above comment has typo. Should have been
> > > get_user_pages_longterm return -EOPNOTSUPP.
> > > 
> > > > You only need to switch to the long term version if the duration of
> > > > the GUP is under control of user space - ie it may last forever.
> > > > 
> > > > Short duration pins in the kernel do not need long term.
> > > > 
> > > Thats true but the intention here is to use the long term version
> > > which does check for the FS DAX memory. Instead of calling direct
> > > accessor to check DAX memory region, longterm version of the API
> > > is used
> > > 
> > > > At a minimum the commit message needs re-writing to properly explain
> > > > the motivation here.
> > > > 
> > > Commit is actually trying to describe the motivation describing more of
> > > issues of not making the call fail. The code comment typo was
> > > misleading.
> > 
> > Every single sentence in the commit message is wrong
> > 
> I will rewrite commit message but can you please comment on other
> questions above. GUP long term was used to detect whether its
> fs_dax memory which could be misleading since the RDS MRs are
> short lived. Do you want us to use accessor instead to check
> if its FS DAX memory?

Why would you need to detect FS DAX memory? GUP users are not supposed
to care.

GUP is supposed to work just 'fine' - other than the usual bugs we
have with GUP and any FS backed memory.

Jason