Re: [PATCH 1/9] exofs: osd Swiss army knife

Pavel Machek
2009-01-05T09:36:26Z

> Pavel Machek wrote:
> > On Sun 2009-01-04 10:43:09, Boaz Harrosh wrote:
> >> Pavel Machek wrote:
> >>> Hi!
> >>>
> >>>>> +#ifdef ARCH_HAS_ATOMIC_UNSIGNED
> >>>> This doesn't exist, and it would be fairly bad to introduce it. Please
> >>>> kill the ifdefs.
> >>>>
> >>>>> +typedef unsigned exofs_iflags_t;
> >>>>> +#else
> >>>>> +typedef unsigned long exofs_iflags_t;
> >>>>> +#endif
> >>>> Then please kill the typedef altogether and replace it with `unsigned
> >>>> long' everywhere
> >>> Hmmm.. .and at a note somewhere that we assume unsigned long to be atomic...?
> >>>
> >> I think I'll just use unsigned. It's more then enough I'm not using more then 3
> >> bits for now. Is unsigned workable for all ARCHs?
>
> <added>
> > /*
> > * our extension to the in-memory inode
> > */
> > struct exofs_i_info {
> > unsigned long i_flags; /* various atomic flags */
> <snip>
> >
> > /*
> > * our inode flags
> > */
> > #define OBJ_2BCREATED 0 /* object will be created soon*/
> > #define OBJ_CREATED 1 /* object has been created on the osd*/
> >
> > static inline int obj_2bcreated(struct exofs_i_info *oi)
> > {
> > return test_bit(OBJ_2BCREATED, &(oi->i_flags));
> > }
> >
> > static inline void set_obj_2bcreated(struct exofs_i_info *oi)
> > {
> > set_bit(OBJ_2BCREATED, &(oi->i_flags));
> > }
> >
> > static inline int obj_created(struct exofs_i_info *oi)
> > {
> > return test_bit(OBJ_CREATED, &(oi->i_flags));
> > }
> >
> > static inline void set_obj_created(struct exofs_i_info *oi)
> > {
> > set_bit(OBJ_CREATED, &(oi->i_flags));
> > }
> </added>
>
> >
> > Please just use atomic_t.
> >
> > (see "atomics: document that linux expects certain atomic behaviour"
> > thread for discussion)
> > Pavel
>
> I have a problem with this. The context of i_flags is to be used with
> set_bit() and test_bit(). In some ARCHs like x86_64 they take an
> "unsigned long *" in most others they take a "void *" and cast internally
> to a "u32 *". (for x86_64 I must use "unsigned long", anything else warns)
>
> I think if I declare "unsigned long" but only use 32 bits flags then
> I should be in the clear with ALL archs, I'll see if that works once
> this code sits in linux-next. (That's real ugly I think)
>
> Is set_bit() and test_bit() should only be used from arch/ code? What
> can regular kernel code use?

I believe using test_bit/set_bit on first 32 bits of unsigned long is
okay and portable. Just don't call it atomic :-).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html