Re: [Nfs-ganesha-devel] Stacked FSALs and fsal_export parameters and op_ctx

2017-12-09 Thread William Allen Simpson 

On 12/8/17 10:13 AM, Matt Benjamin wrote:

I'd like to see this use of TLS as a "hidden parameter" replaced
regardless.  It has been a source of bugs, and locks us into a
pthreads execution model I think needlessly.


With future async FSAL calls, it's going to stop working.

We already have a svc_req and now clnt_req.  That's what will be
returned on the new thread.  So it behooves us to stash the op_ctx
pointer in them (as a void *).

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

Re: [Nfs-ganesha-devel] Stacked FSALs and fsal_export parameters and op_ctx

2017-12-08 Thread Daniel Gryniewicz 

fsal_export should probably be set anywhere gsh_export is set.

Daniel

On 12/07/2017 07:54 PM, Frank Filz wrote:

Stacked FSALs often depend on op_ctx->fsal_export being set.

We also have lots of FSAL methods that take the fsal_export as a parameter.

I wonder if we would be better off removing the fsal_export parameters in
almost all cases, and instead expecting op_ctx->fsal_export to be set?

One danger is that if an FSAL method is called for a different export than
op_ctx->fsal_export, the subcall macros will end up changing
op_ctx->fsal_export and break the caller... It would be better that the
caller assure that op_ctx is properly set up (and save the current
op_ctx->fsal_export if necessary).

In any case, we probably need to audit anywhere op_ctx->fsal_export is not
set. I see that RQUOTA does not set it...

One advantage of making sure op_ctx->fsal_export is always set in the upper
layers is that it should ALSO set op_ctx->ctx_export.

We should also check for any place where op_ctx is NULL. We have
subcall_shutdown_raw that assumes op_ctx might not be set, and the only
place it is used is in calling sub_export release, with the result that that
was not actually working right in the DBUS unexport case where there wasn't
a proper op_ctx (though I wonder if that's also why shutdown reports an
extra ref to FSAL_PSEUDO, is there some point in shutdown where we don't
have an op_ctx?).

Frank


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel




--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

Re: [Nfs-ganesha-devel] Stacked FSALs and fsal_export parameters and op_ctx

2017-12-08 Thread Daniel Gryniewicz 
I'm on the opposite end.  I'm not in favor of passing op_ctx needlessly 
to every function in Ganesha.


Any useful threading system must have some form of TLS.

Daniel

On 12/08/2017 10:13 AM, Matt Benjamin wrote:

I'd like to see this use of TLS as a "hidden parameter" replaced
regardless.  It has been a source of bugs, and locks us into a
pthreads execution model I think needlessly.

Matt

On Fri, Dec 8, 2017 at 10:07 AM, Frank Filz  wrote:

On 12/7/17 7:54 PM, Frank Filz wrote:

Stacked FSALs often depend on op_ctx->fsal_export being set.

We also have lots of FSAL methods that take the fsal_export as a

parameter.



The latter sounds better.

Now that we know every single thread local storage access involves a hidden
lock/unlock sequence in glibc "magically" invoked by the linker, it would be
better to remove as many TLS references as possible!

After all, too many lock/unlock are a real performance issue.

Perhaps we should pass op_ctx as the parameter instead.


I thought the lock was only to create the TLS variable, and not on every 
reference.

Frank


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel







--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

Re: [Nfs-ganesha-devel] Stacked FSALs and fsal_export parameters and op_ctx

2017-12-08 Thread Matt Benjamin 
I'd like to see this use of TLS as a "hidden parameter" replaced
regardless.  It has been a source of bugs, and locks us into a
pthreads execution model I think needlessly.

Matt

On Fri, Dec 8, 2017 at 10:07 AM, Frank Filz  wrote:
>> On 12/7/17 7:54 PM, Frank Filz wrote:
>> > Stacked FSALs often depend on op_ctx->fsal_export being set.
>> >
>> > We also have lots of FSAL methods that take the fsal_export as a
>> parameter.
>> >
>> The latter sounds better.
>>
>> Now that we know every single thread local storage access involves a hidden
>> lock/unlock sequence in glibc "magically" invoked by the linker, it would be
>> better to remove as many TLS references as possible!
>>
>> After all, too many lock/unlock are a real performance issue.
>>
>> Perhaps we should pass op_ctx as the parameter instead.
>
> I thought the lock was only to create the TLS variable, and not on every 
> reference.
>
> Frank
>
>
> ---
> This email has been checked for viruses by Avast antivirus software.
> https://www.avast.com/antivirus
>
>
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> Nfs-ganesha-devel mailing list
> Nfs-ganesha-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel



-- 

Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-821-5101
fax.  734-769-8938
cel.  734-216-5309

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

Re: [Nfs-ganesha-devel] Stacked FSALs and fsal_export parameters and op_ctx

2017-12-08 Thread Frank Filz 
> On 12/7/17 7:54 PM, Frank Filz wrote:
> > Stacked FSALs often depend on op_ctx->fsal_export being set.
> >
> > We also have lots of FSAL methods that take the fsal_export as a
> parameter.
> >
> The latter sounds better.
> 
> Now that we know every single thread local storage access involves a hidden
> lock/unlock sequence in glibc "magically" invoked by the linker, it would be
> better to remove as many TLS references as possible!
> 
> After all, too many lock/unlock are a real performance issue.
> 
> Perhaps we should pass op_ctx as the parameter instead.

I thought the lock was only to create the TLS variable, and not on every 
reference.

Frank


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

Re: [Nfs-ganesha-devel] Stacked FSALs and fsal_export parameters and op_ctx

2017-12-08 Thread William Allen Simpson 

On 12/7/17 7:54 PM, Frank Filz wrote:

Stacked FSALs often depend on op_ctx->fsal_export being set.

We also have lots of FSAL methods that take the fsal_export as a parameter.


The latter sounds better.

Now that we know every single thread local storage access involves a
hidden lock/unlock sequence in glibc "magically" invoked by the linker,
it would be better to remove as many TLS references as possible!

After all, too many lock/unlock are a real performance issue.

Perhaps we should pass op_ctx as the parameter instead.

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

[Nfs-ganesha-devel] Stacked FSALs and fsal_export parameters and op_ctx

2017-12-07 Thread Frank Filz 
Stacked FSALs often depend on op_ctx->fsal_export being set.

We also have lots of FSAL methods that take the fsal_export as a parameter.

I wonder if we would be better off removing the fsal_export parameters in
almost all cases, and instead expecting op_ctx->fsal_export to be set?

One danger is that if an FSAL method is called for a different export than
op_ctx->fsal_export, the subcall macros will end up changing
op_ctx->fsal_export and break the caller... It would be better that the
caller assure that op_ctx is properly set up (and save the current
op_ctx->fsal_export if necessary).

In any case, we probably need to audit anywhere op_ctx->fsal_export is not
set. I see that RQUOTA does not set it...

One advantage of making sure op_ctx->fsal_export is always set in the upper
layers is that it should ALSO set op_ctx->ctx_export.

We should also check for any place where op_ctx is NULL. We have
subcall_shutdown_raw that assumes op_ctx might not be set, and the only
place it is used is in calling sub_export release, with the result that that
was not actually working right in the DBUS unexport case where there wasn't
a proper op_ctx (though I wonder if that's also why shutdown reports an
extra ref to FSAL_PSEUDO, is there some point in shutdown where we don't
have an op_ctx?).

Frank


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel