Re: [Nfs-ganesha-devel] [RFC] change struct state_t to include fsal_fd
On Do, 2017-02-23 at 08:35 -0800, Frank Filz wrote:
> >
> However, don't add state_t to the private fd since the global fd does
> not
> need a state and that would needlessly enlarge every FSAL object
> handle.
>
> Instead make a containing structure:
>
> struct myfsal_state_and_fd {
> struct state_t state;
> struct my_file_descriptor my_fd;
> };
>
> Frank
>
Prepared a patch just covering GPFS.
If that's accepted I could do it for the other FSALs as well.
Swen
>
> ---
> 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] [RFC] change struct state_t to include fsal_fd
> On Do, 2017-02-23 at 04:40 -0500, William Allen Simpson wrote:
> > On 2/23/17 3:43 AM, Swen Schillig wrote:
> > >
> > > while removing some legacy GPFS code I stumbled over this
> > >
> > > my_fd = (struct gpfs_fd *)(state + 1);
> > >
> > > which is probably just copied from an early version to all our
> > > FSALs.
> > >
> > Looks like the state struct is followed by per-FSAL independent data.
> >
> > A better design than this (or a union) would be container_of, so that
> > each FSAL is independent, and needs no centralized union.
>
> True.
> For me, the main thing was the idea of having it as a part of state_t.
> I knew some might object to the split alloc idea, that's why I started the
> discussion with the union.
>
> Anyway, all ideas raised so far are better than what's there, so let's see
what
> else is there to come and maybe we can agree to one then.
Yea, if we change it lets go with container_of. The union above burdens
every FSAL with the largest private fd whether it uses one or not.
container_of doesn't require split allocation.
However, don't add state_t to the private fd since the global fd does not
need a state and that would needlessly enlarge every FSAL object handle.
Instead make a containing structure:
struct myfsal_state_and_fd {
struct state_t state;
struct my_file_descriptor my_fd;
};
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] [RFC] change struct state_t to include fsal_fd
On Do, 2017-02-23 at 04:40 -0500, William Allen Simpson wrote: > On 2/23/17 3:43 AM, Swen Schillig wrote: > > > > while removing some legacy GPFS code I stumbled over this > > > > my_fd = (struct gpfs_fd *)(state + 1); > > > > which is probably just copied from an early version to all our > > FSALs. > > > Looks like the state struct is followed by per-FSAL independent data. > > A better design than this (or a union) would be container_of, so that > each FSAL is independent, and needs no centralized union. True. For me, the main thing was the idea of having it as a part of state_t. I knew some might object to the split alloc idea, that's why I started the discussion with the union. Anyway, all ideas raised so far are better than what's there, so let's see what else is there to come and maybe we can agree to one then. Swen. > -- 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] [RFC] change struct state_t to include fsal_fd
On 2/23/17 3:43 AM, Swen Schillig wrote: > while removing some legacy GPFS code I stumbled over this > > my_fd = (struct gpfs_fd *)(state + 1); > > which is probably just copied from an early version to all our FSALs. > Looks like the state struct is followed by per-FSAL independent data. A better design than this (or a union) would be container_of, so that each FSAL is independent, and needs no centralized union. -- 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] [RFC] change struct state_t to include fsal_fd
On Do, 2017-02-23 at 14:28 +0530, Malahal Naineni wrote:
> That won't work for out of tree fsals, right? Either decouple them
> with a pointer or have a suitable macro to convert. To avoid split
> allocs, a macro would be better.
>
> Regards, Malahal.
> PS: #define GPFS_STATE2FD(state) ((struct gpfs_fd
> *)((state)+sizeof(struct state_t))) # similar for other fsal!
>
Didn't realize that there is support for out of tree FSALs.
But, isn't that the same thing just wrapped a little nicer ?
I simply don't like code fragments with side-effects and I would
consider the removal of the gpfs_fd-memory by doing a free(state)
a side-effect.
However, I agree that this would already be an improvement with very
little effort.
Swen
> On Thu, Feb 23, 2017 at 2:13 PM, Swen Schillig
> wrote:
> >
> > All
> >
> > while removing some legacy GPFS code I stumbled over this
> >
> > my_fd = (struct gpfs_fd *)(state + 1);
> >
> > which is probably just copied from an early version to all our
> > FSALs.
> >
> > Since we're still in dev for 2.5 I'd volunteer and "transfer" this
> > into maybe this
> >
> > struct state_t {
> > ...
> > union {
> > struct gluster_fd gluster;
> > struct ceph_fd fd ceph;
> > struct gpfs_fd gpfs;
> > struct vfs_fd vfs;
> > } fd;
> > }
> >
> > comments ?
> >
> > Swen
> >
> >
> > -
> > -
> > 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] [RFC] change struct state_t to include fsal_fd
That won't work for out of tree fsals, right? Either decouple them
with a pointer or have a suitable macro to convert. To avoid split
allocs, a macro would be better.
Regards, Malahal.
PS: #define GPFS_STATE2FD(state) ((struct gpfs_fd
*)((state)+sizeof(struct state_t))) # similar for other fsal!
On Thu, Feb 23, 2017 at 2:13 PM, Swen Schillig wrote:
> All
>
> while removing some legacy GPFS code I stumbled over this
>
> my_fd = (struct gpfs_fd *)(state + 1);
>
> which is probably just copied from an early version to all our FSALs.
>
> Since we're still in dev for 2.5 I'd volunteer and "transfer" this
> into maybe this
>
> struct state_t {
> ...
> union {
> struct gluster_fd gluster;
> struct ceph_fd fd ceph;
> struct gpfs_fd gpfs;
> struct vfs_fd vfs;
> } fd;
> }
>
> comments ?
>
> Swen
>
>
> --
> 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
