[PATCH 1/2] cpukit/jffs2: Set extracted nodes off-chain
Sebastian Huber
sebastian.huber at embedded-brains.de
Fri Sep 22 05:55:27 UTC 2023
On 21.09.23 18:48, Kinsey Moore wrote:
> On Thu, Sep 21, 2023 at 10:23 AM Sebastian Huber
> <sebastian.huber at embedded-brains.de
> <mailto:sebastian.huber at embedded-brains.de>> wrote:
>
> On 21.09.23 17:06, Kinsey Moore wrote:
> > On Thu, Sep 21, 2023 at 10:01 AM Sebastian Huber
> > <sebastian.huber at embedded-brains.de
> <mailto:sebastian.huber at embedded-brains.de>
> > <mailto:sebastian.huber at embedded-brains.de
> <mailto:sebastian.huber at embedded-brains.de>>> wrote:
> >
> > On 21.09.23 16:52, Kinsey Moore wrote:
> > > On Thu, Sep 21, 2023 at 9:47 AM Sebastian Huber
> > > <sebastian.huber at embedded-brains.de
> <mailto:sebastian.huber at embedded-brains.de>
> > <mailto:sebastian.huber at embedded-brains.de
> <mailto:sebastian.huber at embedded-brains.de>>
> > > <mailto:sebastian.huber at embedded-brains.de
> <mailto:sebastian.huber at embedded-brains.de>
> > <mailto:sebastian.huber at embedded-brains.de
> <mailto:sebastian.huber at embedded-brains.de>>>> wrote:
> > >
> > > On 20.09.23 20:35, Kinsey Moore wrote:
> > > [...]
> > > > @@ -1306,8 +1307,22 @@ static void
> process_delayed_work(void)
> > > > while (!rtems_chain_is_tail(&process_work_chain,
> > node)) {
> > > > work = (struct delayed_work*) node;
> > > > rtems_chain_node* next_node =
> > rtems_chain_next(node);
> > > > +
> > > > + /*
> > > > + * Don't leave extracted node exposed
> to other
> > > operations
> > > > + * under RTEMS_DEBUG
> > > > + */
> > > > +#ifdef RTEMS_DEBUG
> > > > + mutex_lock(&delayed_work_mutex);
> > > > +#endif
> > > > rtems_chain_extract(node);
> > > > +#ifdef RTEMS_DEBUG
> > > > + node->next = node;
> > > > + mutex_unlock(&delayed_work_mutex);
> > > > +#endif
> > > > +
> > > > work->callback(&work->work);
> > > > + rtems_chain_set_off_chain(node);
> > > > node = next_node;
> > > > }
> > > > }
> > >
> > > This change makes no sense to me. The code should work
> > regardless if
> > > RTEMS_DEBUG is defined or not.
> > >
> > >
> > > RTEMS_DEBUG introduces a behavioral change in
> > rtems_chain_extract() such
> > > that extracted nodes are automatically set off-chain. When
> > RTEMS_DEBUG
> > > is not set, node->next is left untouched. This has to be
> managed
> > because
> > > this code needs the node to remain on-chain so that it is not
> > re-queued
> > > during the operation.
> >
> > Yes, but while a node is on a chain you must not call
> > rtems_chain_set_off_chain(). If you want to use the off-chain
> state,
> > then you have to use this:
> >
> > rtems_chain_extract(node);
> > rtems_chain_set_off_chain(node);
> >
> > or
> >
> > rtems_chain_extract_unprotected(node);
> > rtems_chain_set_off_chain(node);
> >
> > The automatic set off-chain in RTEMS_DEBUG is just to ensure
> that basic
> > chain operations are used in the right state.
> >
> >
> > Yes, there is no behavior here where rtems_chain_set_off_chain() is
> > being called on a node that is still in a chain. This section of
> code is
> > entirely managing the side-effect of RTEMS_DEBUG that sets the
> node in
> > the off-chain state post-extraction. In this case, that
> side-effect is
> > undesirable and so a lock is in place to prevent that temporary
> > side-effect from leaking to other parts of the system since all
> other
> > off-chain checks are behind the same lock.
>
> Ok, so you want to delay the set-off state visibility in case
> RTEMS_DEBUG is defined. This is the first place in the code base which
> needs this behaviour.
>
>
> Would it be more acceptable to always lock it and ensure that it's
> recognized as on-chain instead of checking for RTEMS_DEBUG?
>
>
> We basically don't use internal locking in JFFS2 in RTEMS. Why can't
> you
> use the global file system lock of the JFFS2 instance to work carry out
> this delayed work?
>
>
> The JFFS2 instance to which the delayed work belongs is a detail
> internal to the delayed work itself. Reaching that information is
> possible, but would be fragile. The other option I had in mind was to
> create one delayed work thread per JFFS2 FS instance so that the delayed
> work processor can know reliably which JFFS2 instance it is working on,
> but that seemed wasteful.
I would do the following.
1. The delayed work support uses a mutex D and a condition variable C
used with D.
2. Add a queue for the delayed work to the fs information and a node to
register the info in the delayed work support.
3. The first delayed work request of a JFFS2 instance registers the fs
information in the delayed work support and uses C to signal the work to
the delayed work task.
4. Further requests just get enqueued and signaled using D and C.
5. When a instance is unmounted, drain the delayed work queue using D and C.
The delayed work uses the fs info mutex to protect the work. You need
also reference count for the fs info to control the work and the drain
during unmount.
--
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber at embedded-brains.de
phone: +49-89-18 94 741 - 16
fax: +49-89-18 94 741 - 08
Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
More information about the devel
mailing list