Greg's commit c60b71787982cefcf9fa09aa281fa8c4c685d557 inadvertantly broke
Ocfs2 userspace ABI, so I have a rather high priority single line patch from
Joel to fix things up for you to pull. A copy of the patch is attached to
the bottom of this e-mail. Embarassingly enough, I missed this while acking
the patch late last week :(
Please pull from 'upstream-linus' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/mfasheh/ocfs2.git upstream-linus
to receive the following updates:
fs/ocfs2/cluster/sys.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
Joel Becker (1):
ocfs2: Fix userspace ABI breakage in sysfs
From: Joel Becker <[email protected]>
ocfs2: Fix userspace ABI breakage in sysfs
The userspace ABI of ocfs2's internal cluster stack (o2cb) was broken by
commit c60b71787982cefcf9fa09aa281fa8c4c685d557 "kset: convert ocfs2 to
use kset_create". Specifically, the '/sys/o2cb' kset was moved to
'/sys/fs/o2cb'. This breaks all ocfs2 tools and renders the
filesystem unmountable.
This fix moves '/sys/o2cb' back where it belongs.
Signed-off-by: Joel Becker <[email protected]>
Signed-off-by: Mark Fasheh <[email protected]>
---
fs/ocfs2/cluster/sys.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/ocfs2/cluster/sys.c b/fs/ocfs2/cluster/sys.c
index a4b0773..0c095ce 100644
--- a/fs/ocfs2/cluster/sys.c
+++ b/fs/ocfs2/cluster/sys.c
@@ -64,7 +64,7 @@ int o2cb_sys_init(void)
{
int ret;
- o2cb_kset = kset_create_and_add("o2cb", NULL, fs_kobj);
+ o2cb_kset = kset_create_and_add("o2cb", NULL, NULL);
if (!o2cb_kset)
return -ENOMEM;
--
1.5.3.6
On Mon, Jan 28, 2008 at 07:33:07PM -0800, Mark Fasheh wrote:
> Greg's commit c60b71787982cefcf9fa09aa281fa8c4c685d557 inadvertantly broke
> Ocfs2 userspace ABI, so I have a rather high priority single line patch from
> Joel to fix things up for you to pull. A copy of the patch is attached to
> the bottom of this e-mail. Embarassingly enough, I missed this while acking
> the patch late last week :(
>
> Please pull from 'upstream-linus' branch of
> git://git.kernel.org/pub/scm/linux/kernel/git/mfasheh/ocfs2.git upstream-linus
>
> to receive the following updates:
>
> fs/ocfs2/cluster/sys.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> Joel Becker (1):
> ocfs2: Fix userspace ABI breakage in sysfs
This is fine with me, for now.
> From: Joel Becker <[email protected]>
>
> ocfs2: Fix userspace ABI breakage in sysfs
>
> The userspace ABI of ocfs2's internal cluster stack (o2cb) was broken by
> commit c60b71787982cefcf9fa09aa281fa8c4c685d557 "kset: convert ocfs2 to
> use kset_create". Specifically, the '/sys/o2cb' kset was moved to
> '/sys/fs/o2cb'. This breaks all ocfs2 tools and renders the
> filesystem unmountable.
>
> This fix moves '/sys/o2cb' back where it belongs.
"belongs" is pretty odd here. This is a filesystem specific thing,
right? Why not put it in /sys/fs/ then?
And yes, I understand about legacy userspace tools, that's why I have no
objection to it going back. But you can put it in both places (with a
symlink) and change your userspace code, and in a year or so, drop the
symlink, right?
And please please please please document stuff like this, and all of the
different files you have in this subdirectory in Documentation/ABI/ so
those of us who are trying to figure out the code (and there's still
parts of the kobject usage I'm pretty sure is not correct) can have a
chance to understand exactly how this stuff is being used and expected
to work.
thanks,
greg k-h
On Mon, Jan 28, 2008 at 09:08:04PM -0800, Greg KH wrote:
> > Joel Becker (1):
> > ocfs2: Fix userspace ABI breakage in sysfs
>
> This is fine with me, for now.
Great, thanks.
> > From: Joel Becker <[email protected]>
> >
> > ocfs2: Fix userspace ABI breakage in sysfs
> >
> > The userspace ABI of ocfs2's internal cluster stack (o2cb) was broken by
> > commit c60b71787982cefcf9fa09aa281fa8c4c685d557 "kset: convert ocfs2 to
> > use kset_create". Specifically, the '/sys/o2cb' kset was moved to
> > '/sys/fs/o2cb'. This breaks all ocfs2 tools and renders the
> > filesystem unmountable.
> >
> > This fix moves '/sys/o2cb' back where it belongs.
>
> "belongs" is pretty odd here. This is a filesystem specific thing,
> right? Why not put it in /sys/fs/ then?
We had it there before /sys/fs and as has been noted, it's ABI so we can't
change it right away. In theory, it's actually outside the fs, but in
reality it's pretty tied to Ocfs2, so I have no objection to the idea of it
being eventually moved there.
> And yes, I understand about legacy userspace tools, that's why I have no
> objection to it going back. But you can put it in both places (with a
> symlink) and change your userspace code, and in a year or so, drop the
> symlink, right?
Yeah, that sounds entirely reasonable. It shouldn't be too hard for us to
fix up ocfs2-tools to look in both places. So long as there's enough lead
time for users to upgrade their toolchain (we can do releases for all
branches of ocfs2-tools, make annoucements on lists, etc), I think the
impact shouldn't be too bad.
> And please please please please document stuff like this, and all of the
> different files you have in this subdirectory in Documentation/ABI/ so
> those of us who are trying to figure out the code (and there's still
> parts of the kobject usage I'm pretty sure is not correct) can have a
> chance to understand exactly how this stuff is being used and expected
> to work.
No problem. I'll get us some patches to symlink things, and add docs in
Documentation/ABI/ explaining how Ocfs2 and userspace communicate. In the
future, as we add ABI it'll be documented there.
--Mark
--
Mark Fasheh
Principal Software Developer, Oracle
[email protected]
On Mon, Jan 28, 2008 at 09:08:04PM -0800, Greg KH wrote:
> And please please please please document stuff like this, and all of the
> different files you have in this subdirectory in Documentation/ABI/ so
Huh, I didn't know Documentation/ABI existed. That would
certainly help in the future.
> those of us who are trying to figure out the code (and there's still
> parts of the kobject usage I'm pretty sure is not correct) can have a
ocfs2 kobject usage, or other folks'? If there's anything in
the ocfs2 usage that you are unsure of, feel free to ask!
Joel
--
"Also, all of life's big problems include the words 'indictment' or
'inoperable.' Everything else is small stuff."
- Alton Brown
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Mon, Jan 28, 2008 at 09:58:25PM -0800, Mark Fasheh wrote:
> On Mon, Jan 28, 2008 at 09:08:04PM -0800, Greg KH wrote:
> > And please please please please document stuff like this, and all of the
> > different files you have in this subdirectory in Documentation/ABI/ so
> > those of us who are trying to figure out the code (and there's still
> > parts of the kobject usage I'm pretty sure is not correct) can have a
> > chance to understand exactly how this stuff is being used and expected
> > to work.
>
> No problem. I'll get us some patches to symlink things, and add docs in
> Documentation/ABI/ explaining how Ocfs2 and userspace communicate. In the
> future, as we add ABI it'll be documented there.
Great, that would be wonderful.
thanks,
greg k-h
On Mon, Jan 28, 2008 at 11:44:02PM -0800, Joel Becker wrote:
> On Mon, Jan 28, 2008 at 09:08:04PM -0800, Greg KH wrote:
> > And please please please please document stuff like this, and all of the
> > different files you have in this subdirectory in Documentation/ABI/ so
>
> Huh, I didn't know Documentation/ABI existed. That would
> certainly help in the future.
Yeah, not many people know about it, despite being present for over a
year now :(
So I'm starting to poke people to fix this. I'd like to see all new
sysfs attributes (and syscalls and other stuff that is an ABI) be
documented here. As well as creating the information for the ABI we
currently have, but I know that will take longer.
> > those of us who are trying to figure out the code (and there's still
> > parts of the kobject usage I'm pretty sure is not correct) can have a
>
> ocfs2 kobject usage, or other folks'? If there's anything in
> the ocfs2 usage that you are unsure of, feel free to ask!
Ok, great. Here's a few questions:
- ocfs2/cluster/sys.c creates a single kset, o2cb, that is in
/sys/o2cb (and I had moved it to /sys/fs/o2cb/) In that directory
it seems there is only 1 file, O2NM_API_VERSION. Is that really all
that goes into that directory? If so, this can be cleaned up even
further and only a single kobject is needed, a kset is overkill.
- that kset is then passed to the mlog_sys_init() file to chain
another subdirectory off of this. Here's where the meat of the
sysfs files are. You use a custom kset and show/store operations to
describe some bit fields? You never use the kobject here, but only
go off of the name of the attribute file, which is fine. But again,
using a ktype/kset is way overkill for this. It can be all
simplified to only have 1 kobject for the directory, and then the
individual attributes can be a kobj_attribute.
Actually that wasn't too bad. It was gfs2 that I was thinking about
with regard to totally strange kobject abuse, I'll go poke those
developers about it while I'm remembering :)
thanks,
greg k-h
On Tue, Jan 29, 2008 at 10:54:48AM -0800, Greg KH wrote:
> > ocfs2 kobject usage, or other folks'? If there's anything in
> > the ocfs2 usage that you are unsure of, feel free to ask!
>
> Ok, great. Here's a few questions:
>
> - ocfs2/cluster/sys.c creates a single kset, o2cb, that is in
> /sys/o2cb (and I had moved it to /sys/fs/o2cb/) In that directory
> it seems there is only 1 file, O2NM_API_VERSION. Is that really all
> that goes into that directory? If so, this can be cleaned up even
> further and only a single kobject is needed, a kset is overkill.
The kset exists so we can put the logmask directory underneath
it, as you notice in the next item.
> - that kset is then passed to the mlog_sys_init() file to chain
> another subdirectory off of this. Here's where the meat of the
> sysfs files are. You use a custom kset and show/store operations to
> describe some bit fields? You never use the kobject here, but only
> go off of the name of the attribute file, which is fine. But again,
> using a ktype/kset is way overkill for this. It can be all
> simplified to only have 1 kobject for the directory, and then the
> individual attributes can be a kobj_attribute.
Well, what's a kobj_attribute do differently? I mean, logmask
is one object, distinct from the o2cb toplevel, and it has multiple
attributes. That's pretty standard sysfs, no?
Oh, I see, kobj_attribute wraps struct attribute the way we do
with "mlog_attribute". Pretty much everyone did it themselves back in
the day, with a set of hand-built "turn struct attribute into struct
foo_attribute" version of show() and store(). But then you have ot wrap
that in our own attribute anyway...well, not for the mlog one, I don't
think, as it keys off the name. So perhaps we could remove "struct
mlog_attribute" and replace it with "struct kobj_attribute" for a
zero-sum change. Not pressing, but certainly not a problem.
But I'm still not understanding what you mean by "1 kobject for
the directory". We have one kobject for the "logmask" directory, and
then attributes for each log type. I don't see how that would change.
Are you suggesting that we move the log attributes up one level,
eliminating the "logmask" subdir? That is, "/sys/fs/o2cb/logmask/DLM"
becomes "/sys/fs/o2cb/DLM"? That won't work, because then "ls
/sys/fs/o2cb/logmask" is no longer a valid way to discover all the log
masks - "interface_revision" isn't a log mask :-)
Or do you mean something else?
Joel
--
"To announce that there must be no criticism of them president, or
that we are to stand by the president, right or wrong, is not only
unpatriotic and servile, but is morally treasonable to the American
public."
- Theodore Roosevelt
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Tue, Jan 29, 2008 at 02:18:58PM -0800, Joel Becker wrote:
> But I'm still not understanding what you mean by "1 kobject for
> the directory". We have one kobject for the "logmask" directory, and
> then attributes for each log type. I don't see how that would change.
Oh, wait, I'm an idiot. It does look like its a kset. If we
set the ktype to have the ops and attrs, just like we do, but use a
kobj, would that make you happier?
Joel
--
"The whole problem with the world is that fools and fanatics are always
so certain of themselves, and wiser people so full of doubts."
- Bertrand Russell
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127