On Wednesday 18 June 2014 at 13:49:30, Edward Shishkin wrote:
> On 06/18/2014 11:55 AM, Ivan Shapovalov wrote:
> > On Wednesday 18 June 2014 at 03:41:04, Edward Shishkin wrote:
> >> On 06/17/2014 10:47 PM, Ivan Shapovalov wrote:
> >>> On Tuesday 17 June 2014 at 22:31:36, Ivan Shapovalov wrote:
> >>>> On Tuesday 17 June 2014 at 12:29:53, Edward Shishkin wrote:
> >>>>> [...]
> >>>>>
> >>>>> Yup.
> >>>>> So, if discard is on, we work with 2 lists (delete_set,
> >>>>> delete_set_for_wander).
> >>>>> If discard is off, we work with one blocknr set..
> >>>> Good. So I'll do roughly following for v5:
> >>>> - rename discard_set_* to block_list_* and split off these definitions
> >>>> - write a family of reiser4_atom_dset_*() (log_deferred, log_immediate,
> >>>> apply_deferred, merge, init, destroy) which will encapsulate discard/nodiscard
> >>>> check and operate on correct lists (blocknr_set vs block_list)
> >>>> - call reiser4_atom_dset_{init,destroy,merge}() from respective functions
> >>>> - call reiser4_atom_dset_log_{deferred,immediate}() from reiser4_dealloc_blocks()
> >>>> - call reiser4_atom_dset_apply_deferred() from reiser4_post_commit_hook()
> >>>> - directly manipulate the block lists from discard_atom(), checking that we
> >>>> indeed have discard enabled
> >>>>
> >>>> Is this OK?
> >>> BTW, with txn_atoms there is a locking idiom involving E_REPEAT loops.
> >>>
> >>> Is it fine to implement a
> >>> current_atom_dset_log_...(...) // E_REPEAT loop inside
> >>> instead of
> >>> atom_dset_log_...(txn_atom* atom, ...)
> >> IMHO it is not needed.
> >>
> >> atom = get_current_atom_locked();
> >> atom_dset_{defer, immed}_add_extent(atom, ...); // <-- the loop is here
> >> spin_unlock_atom(atom);
> > IIUC, the point of the E_REPEAT loop is that memory allocations must be done
> > with atom spinlock released, and once released,
>
>
> You are right, we need something similar for the lists. Otherwise,
> deadlocks are possible (they will appear on high-load systems).
> Not necessarily in the first edition: first make sure that the base
> stuff works.
It's cheap, there is no need to delay this until some ephemeral "final
imlementation". I just wanted to make sure it's OK per coding style to
implement functions like "current_atom_XXX()" which internally do
get_current_atom_locked() and all that stuff. (Or not OK, in which case
I'll push this loop out of the function.)
Thanks,
--
Ivan Shapovalov / intelfx /
>
>
> > the atom may "disappear", so
> > it is unsafe to use the same pointer again. Hence there is a loop of
> >
> > void *data = NULL;
> >
> > do {
> > atom = get_current_atom_locked();
> > ret = blocknr_set_add_extent(atom, &atom->delete_set, &data, ...);
> > } while (ret == -E_REPEAT);
> >
> > where `data` is a piece of "state" saved across the two blocknr_set_add_extent()
> > calls. On first iteration of the loop, atom lock is released, memory is
> > allocated and pointer is stored in `data`, and -E_REPEAT is returned.
> > On second iteration of the loop, the previously allocated memory is accessed
> > and the atom is modified under its spinlock.
> >
> Attachment:
signature.asc
Description: This is a digitally signed message part.
