[PATCH 0/13] sysfs lazification.

Quoting Eric W. Biederman (ebiederm@xmission.com):
> From: Eric W. Biederman <ebiederm.TakeThisOut@xmission.com>
>
> Cleanly separate the work that is specific to setting the
> attributes of a sysfs_dirent from what is needed to update
> the attributes of a vfs inode.
>
> Additionally grab the sysfs_mutex to keep any nasties from
> surprising us when updating the sysfs_dirent.
>
> Acked-by: Tejun Heo <tj.TakeThisOut@kernel.org>
> Signed-off-by: Eric W. Biederman <ebiederm.TakeThisOut@aristanetworks.com>

Oh, sorry, guess i never did

Acked-by: Serge Hallyn <serue.TakeThisOut@us.ibm.com>

> ---
> fs/sysfs/inode.c | 52 ++++++++++++++++++++++++++++++++--------------------
> fs/sysfs/sysfs.h | 1 +
> 2 files changed, 33 insertions(+), 20 deletions(-)
>
> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> index fed7a74..fccfb55 100644
> --- a/fs/sysfs/inode.c
> +++ b/fs/sysfs/inode.c
> @@ -64,30 +64,15 @@ struct sysfs_inode_attrs *sysfs_init_inode_attrs(struct sysfs_dirent *sd)
>
> return attrs;
> }
> -int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
> +
> +int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
> {
> - struct inode * inode = dentry->d_inode;
> - struct sysfs_dirent * sd = dentry->d_fsdata;
> struct sysfs_inode_attrs *sd_attrs;
> struct iattr *iattrs;
> unsigned int ia_valid = iattr->ia_valid;
> - int error;
> -
> - if (!sd)
> - return -EINVAL;
>
> sd_attrs = sd->s_iattr;
>
> - error = inode_change_ok(inode, iattr);
> - if (error)
> - return error;
> -
> - iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */
> -
> - error = inode_setattr(inode, iattr);
> - if (error)
> - return error;
> -
> if (!sd_attrs) {
> /* setting attributes for the first time, allocate now */
> sd_attrs = sysfs_init_inode_attrs(sd);
> @@ -110,12 +95,39 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
> iattrs->ia_ctime = iattr->ia_ctime;
> if (ia_valid & ATTR_MODE) {
> umode_t mode = iattr->ia_mode;
> -
> - if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> - mode &= ~S_ISGID;
> iattrs->ia_mode = sd->s_mode = mode;
> }
> }
> + return 0;
> +}
> +
> +int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
> +{
> + struct inode * inode = dentry->d_inode;
> + struct sysfs_dirent * sd = dentry->d_fsdata;
> + int error;
> +
> + if (!sd)
> + return -EINVAL;
> +
> + error = inode_change_ok(inode, iattr);
> + if (error)
> + return error;
> +
> + iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */
> + if (iattr->ia_valid & ATTR_MODE) {
> + if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> + iattr->ia_mode &= ~S_ISGID;
> + }
> +
> + error = inode_setattr(inode, iattr);
> + if (error)
> + return error;
> +
> + mutex_lock(&sysfs_mutex);
> + error = sysfs_sd_setattr(sd, iattr);
> + mutex_unlock(&sysfs_mutex);
> +
> return error;
> }
>
> diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
> index af4c4e7..a96d967 100644
> --- a/fs/sysfs/sysfs.h
> +++ b/fs/sysfs/sysfs.h
> @@ -155,6 +155,7 @@ static inline void __sysfs_put(struct sysfs_dirent *sd)
> */
> struct inode *sysfs_get_inode(struct sysfs_dirent *sd);
> void sysfs_delete_inode(struct inode *inode);
> +int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr *iattr);
> int sysfs_setattr(struct dentry *dentry, struct iattr *iattr);
> int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
> size_t size, int flags);
> --
> 1.6.5.2.143.g8cc62
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Mon, Nov 9, 2009 at 6:14 AM, Serge E. Hallyn <serue.TakeThisOut@us.ibm.com> wrote:
> Quoting Tejun Heo (tj@kernel.org):
>> Hello,
>>
>> Eric W. Biederman wrote:
>> > It isn't what I want but it is what the VFS requires.  If let the vfs
>> > continue on it's delusional state we will leak the vfs mount and
>> > everything mounted on top of it, with no way to remove the mounts.
>>
>> This is caused by not having any way to prevent deletion on
>> directories with submounts, right?  How does other distributed
>> filesystems deal with directories with submounts going away underneath
>> it?
>
> Ooooh.  I see, I was thinking only about the rename case, and forgot
> this was the path for deleted files, too.  For the rename case it
> should be ok to let the dentry be put since the submounts will be
> accessible at the new location, right?  Should that be handled
> separately?

No in the rename case it isn't ok to let the dentry be discarded put as mounts
are implemented using a hash of the struct dentry's address, and if you aren't
the mount point you are referenced as d_parent.

For rename I am slightly better than NFS. sysfs does not support hard
links so if I am looking up the new name I can look for a preexisting
dentry for my
inode and if I find one I call d_move on it to lazily perform the rename.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Greg KH <greg DeleteThis @kroah.com> writes:

> On Tue, Nov 03, 2009 at 03:53:56AM -0800, Eric W. Biederman wrote:
>>
>> The sysfs code updates the vfs caches immediately when the sysfs data
>> structures change causing a lot of unnecessary complications. The
>> following patchset untangles that beast. Allowing for simpler
>> more straight forward code, the removal of a hack from the vfs
>> to support sysfs, and human comprehensible locking on sysfs.
>>
>> Most of these patches have already been reviewed and acked from the
>> last time I had time to work on sysfs.
>>
>> In net the patches look like:
>
> Can you resend these based on the review that you just got, with the new
> acks and changes so that I can apply them?

Are you going to apply them soon? If you don't have time I can start
a sysfs tree and just ask Linus to pull them.

Eric



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, Nov 17, 2009 at 01:11:22AM -0800, Eric W. Biederman wrote:
> Greg KH <greg.DeleteThis@kroah.com> writes:
>
> > On Tue, Nov 03, 2009 at 03:53:56AM -0800, Eric W. Biederman wrote:
> >>
> >> The sysfs code updates the vfs caches immediately when the sysfs data
> >> structures change causing a lot of unnecessary complications. The
> >> following patchset untangles that beast. Allowing for simpler
> >> more straight forward code, the removal of a hack from the vfs
> >> to support sysfs, and human comprehensible locking on sysfs.
> >>
> >> Most of these patches have already been reviewed and acked from the
> >> last time I had time to work on sysfs.
> >>
> >> In net the patches look like:
> >
> > Can you resend these based on the review that you just got, with the new
> > acks and changes so that I can apply them?
>
> Are you going to apply them soon? If you don't have time I can start
> a sysfs tree and just ask Linus to pull them.

Well, as the merge window is not open right now, that might be tough to
get Linus to take them at the moment

I was away last week, and will apply them to my tree this week, thanks
for your patience.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Greg KH <greg DeleteThis @kroah.com> writes:

> On Tue, Nov 17, 2009 at 01:11:22AM -0800, Eric W. Biederman wrote:
>> Greg KH <greg DeleteThis @kroah.com> writes:
>>
>> > On Tue, Nov 03, 2009 at 03:53:56AM -0800, Eric W. Biederman wrote:
>> >>
>> >> The sysfs code updates the vfs caches immediately when the sysfs data
>> >> structures change causing a lot of unnecessary complications. The
>> >> following patchset untangles that beast. Allowing for simpler
>> >> more straight forward code, the removal of a hack from the vfs
>> >> to support sysfs, and human comprehensible locking on sysfs.
>> >>
>> >> Most of these patches have already been reviewed and acked from the
>> >> last time I had time to work on sysfs.
>> >>
>> >> In net the patches look like:
>> >
>> > Can you resend these based on the review that you just got, with the new
>> > acks and changes so that I can apply them?
>>
>> Are you going to apply them soon? If you don't have time I can start
>> a sysfs tree and just ask Linus to pull them.
>
> Well, as the merge window is not open right now, that might be tough to
> get Linus to take them at the moment

True.

> I was away last week, and will apply them to my tree this week, thanks
> for your patience.

I'm working on it.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Sat, Nov 07, 2009 at 11:27:04PM -0800, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm RemoveThis @xmission.com>
>
> Cleanly separate the work that is specific to setting the
> attributes of a sysfs_dirent from what is needed to update
> the attributes of a vfs inode.
>
> Additionally grab the sysfs_mutex to keep any nasties from
> surprising us when updating the sysfs_dirent.
>
> Acked-by: Tejun Heo <tj RemoveThis @kernel.org>
> Signed-off-by: Eric W. Biederman <ebiederm RemoveThis @aristanetworks.com>

Due to the extended attr work for sysfs that went into Linus's tree
recently, this patch doesn't apply at all anymore.

I've applied the first 5 to my tree, care to respin the other 10 and
resend them?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Greg KH <greg.RemoveThis@kroah.com> writes:

> On Sat, Nov 07, 2009 at 11:27:04PM -0800, Eric W. Biederman wrote:
>> From: Eric W. Biederman <ebiederm.RemoveThis@xmission.com>
>>
>> Cleanly separate the work that is specific to setting the
>> attributes of a sysfs_dirent from what is needed to update
>> the attributes of a vfs inode.
>>
>> Additionally grab the sysfs_mutex to keep any nasties from
>> surprising us when updating the sysfs_dirent.
>>
>> Acked-by: Tejun Heo <tj.RemoveThis@kernel.org>
>> Signed-off-by: Eric W. Biederman <ebiederm.RemoveThis@aristanetworks.com>
>
> Due to the extended attr work for sysfs that went into Linus's tree
> recently, this patch doesn't apply at all anymore.

I will take a look. I'm scratching my about how that caused a problem
because that patch came out of my tree originally, before the rest of
these patches. So if anything I should have problems in the other direction.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Greg KH <greg.TakeThisOut@kroah.com> writes:

> On Sat, Nov 07, 2009 at 11:27:04PM -0800, Eric W. Biederman wrote:
>> From: Eric W. Biederman <ebiederm.TakeThisOut@xmission.com>
>>
>> Cleanly separate the work that is specific to setting the
>> attributes of a sysfs_dirent from what is needed to update
>> the attributes of a vfs inode.
>>
>> Additionally grab the sysfs_mutex to keep any nasties from
>> surprising us when updating the sysfs_dirent.
>>
>> Acked-by: Tejun Heo <tj.TakeThisOut@kernel.org>
>> Signed-off-by: Eric W. Biederman <ebiederm.TakeThisOut@aristanetworks.com>
>
> Due to the extended attr work for sysfs that went into Linus's tree
> recently, this patch doesn't apply at all anymore.

It looks like the issue is the a trivial context conflict with
sysfs-mark-a-locally-only-used-function-static.patch

Which marks one of those new functions as static.

Here come the patches 6-15 rebased on top of that.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, Nov 20, 2009 at 04:07:11PM -0800, Eric W. Biederman wrote:
> Greg KH <greg RemoveThis @kroah.com> writes:
>
> > On Sat, Nov 07, 2009 at 11:27:04PM -0800, Eric W. Biederman wrote:
> >> From: Eric W. Biederman <ebiederm RemoveThis @xmission.com>
> >>
> >> Cleanly separate the work that is specific to setting the
> >> attributes of a sysfs_dirent from what is needed to update
> >> the attributes of a vfs inode.
> >>
> >> Additionally grab the sysfs_mutex to keep any nasties from
> >> surprising us when updating the sysfs_dirent.
> >>
> >> Acked-by: Tejun Heo <tj RemoveThis @kernel.org>
> >> Signed-off-by: Eric W. Biederman <ebiederm RemoveThis @aristanetworks.com>
> >
> > Due to the extended attr work for sysfs that went into Linus's tree
> > recently, this patch doesn't apply at all anymore.
>
> It looks like the issue is the a trivial context conflict with
> sysfs-mark-a-locally-only-used-function-static.patch

Ah, sorry about that, I should have noticed that, I was looking for a
much harder problem

I'll go queue these up in a bit.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Mon, Nov 30, 2009 at 01:33:37PM -0800, Greg KH wrote:
> On Sat, Nov 07, 2009 at 11:25:03PM -0800, Eric W. Biederman wrote:
> >
> > The sysfs code updates the vfs caches immediately when the sysfs data
> > structures change causing a lot of unnecessary complications. The
> > following patchset untangles that beast. Allowing for simpler
> > more straight forward code, the removal of a hack from the vfs
> > to support sysfs, and human comprehensible locking on sysfs.
> >
> > Most of these patches have already been reviewed and acked from the
> > last time I had time to work on sysfs.
> >
> > This acks have been folded in and the two small bugs found in the
> > previous review have been fixed in the trailing patches (they are
> > minor enough nits that even a bisect that happens to land in the
> > middle should not see sysfs problems).
>
> I've applied all of these to my tree now, and sorry, but something is
> broken pretty badly.
>
> When doing a simple 'ls /sys/class/input/' the process locks up. This
> means that X can't find any input devices, which makes for a bit of a
> problem when wanting to use your mouse or keyboard
>
> Attached is the state of my processes when this happens, if that helps
> out any.
>
> So I'm going to drop all of these from my tree again, as they are not
> ready for merging at this point

In looking at the stuck processes, it seems your last patch was the
problem. Removing that caused things to work again, so I've only
dropped that one.

Next time, please test your patches before submitting them

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Greg KH <greg RemoveThis @kroah.com> writes:

> On Mon, Nov 30, 2009 at 01:33:37PM -0800, Greg KH wrote:
>> On Sat, Nov 07, 2009 at 11:25:03PM -0800, Eric W. Biederman wrote:
>> >
>> > The sysfs code updates the vfs caches immediately when the sysfs data
>> > structures change causing a lot of unnecessary complications. The
>> > following patchset untangles that beast. Allowing for simpler
>> > more straight forward code, the removal of a hack from the vfs
>> > to support sysfs, and human comprehensible locking on sysfs.
>> >
>> > Most of these patches have already been reviewed and acked from the
>> > last time I had time to work on sysfs.
>> >
>> > This acks have been folded in and the two small bugs found in the
>> > previous review have been fixed in the trailing patches (they are
>> > minor enough nits that even a bisect that happens to land in the
>> > middle should not see sysfs problems).
>>
>> I've applied all of these to my tree now, and sorry, but something is
>> broken pretty badly.
>>
>> When doing a simple 'ls /sys/class/input/' the process locks up. This
>> means that X can't find any input devices, which makes for a bit of a
>> problem when wanting to use your mouse or keyboard
>>
>> Attached is the state of my processes when this happens, if that helps
>> out any.
>>
>> So I'm going to drop all of these from my tree again, as they are not
>> ready for merging at this point
>
> In looking at the stuck processes, it seems your last patch was the
> problem. Removing that caused things to work again, so I've only
> dropped that one.
>
> Next time, please test your patches before submitting them

Weird I thought I had tested this.

That last patch to add locking that is only needed for vfs coherency
has certainly seen less testing than the others.

I also remember verify that nfs does the same thing, when in fact
nfs takes inode->i_lock not inode->i_mutex in the same situation.

generic_permission takes no locks so this is really about serializing
writes to the inode. The vfs only takes inode->i_mutex, when calling
notify_change.

So it appears I have stepped into a murky corner of the vfs.

I will take a look and do a bit more testing. At the moment it looks
like a solution to serializing writes to the stat attributes on the inode is
going to be simply holding sysfs_mutex over inode_setattr in
sysfs_setattr. Assuming a solution is needed at all.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/