On Thu, Sep 13, 2007 at 04:22:59PM +0400, Evgeniy Polyakov wrote:
quoted text
> Hi Paul.
> 
> On Mon, Sep 10, 2007 at 03:14:45PM -0700, Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> > > Further TODO list includes:
> > > * implement optional saving of mirroring/linear information on the remote
> > > 	nodes (simple)
> > > * implement netlink based setup (simple)
> > > * new redundancy algorithm (complex)
> > > 
> > > Homepage:
> > > http://tservice.net.ru/~s0mbre/old/?section=projects&item=dst
> > 
> > A couple questions below, but otherwise looks good from an RCU viewpoint.
> > 
> > 							Thanx, Paul
> 
> Thanks for your comments, and sorry for late reply I was at KS/London
> trip.
> > > +	if (--num) {
> > > +		list_for_each_entry_rcu(n, &node->shared, shared) {
> > 
> > This function is called under rcu_read_lock() or similar, right?
> > (Can't tell from this patch.)  It is also OK to call it from under the
> > update-side mutex, of course.
> 
> Actually not, but it does not require it, since entry can not be removed
> during this operations since appropriate reference counter for given node is
> being held. It should not be RCU at all.

Ah!  Yes, it is OK to use _rcu in this case, but should be avoided
unless doing so eliminates duplicate code or some such.  So, agree
with dropping _rcu in this case.

quoted text
> > > +static int dst_mirror_read(struct dst_request *req)
> > > +{
> > > +	struct dst_node *node = req->node, *n, *min_dist_node;
> > > +	struct dst_mirror_priv *priv = node->priv;
> > > +	u64 dist, d;
> > > +	int err;
> > > +
> > > +	req->bio_endio = &dst_mirror_read_endio;
> > > +
> > > +	do {
> > > +		err = -ENODEV;
> > > +		min_dist_node = NULL;
> > > +		dist = -1ULL;
> > > + 
> > > +		/*
> > > +		 * Reading is never performed from the node under resync.
> > > +		 * If this will cause any troubles (like all nodes must be
> > > +		 * resynced between each other), this check can be removed
> > > +		 * and per-chunk dirty bit can be tested instead.
> > > +		 */
> > > +
> > > +		if (!test_bit(DST_NODE_NOTSYNC, &node->flags)) {
> > > +			priv = node->priv;
> > > +			if (req->start > priv->last_start)
> > > +				dist = req->start - priv->last_start;
> > > +			else
> > > +				dist = priv->last_start - req->start;
> > > +			min_dist_node = req->node;
> > > +		}
> > > +
> > > +		list_for_each_entry_rcu(n, &node->shared, shared) {
> > 
> > I see one call to this function that appears to be under the update-side
> > mutex, but I cannot tell if the other calls are safe.  (Safe as in either
> > under the update-side mutex or under rcu_read_lock() and friends.)
> 
> The same here - those processing function are called from
> generic_make_request() from any lock on top of them. Each node is linked
> into the list of the first added node, which reference counter is
> increased in higher layer. Right now there is no way to add or remove
> nodes after array was started, such functionality requires storage tree
> lock to be taken and RCU can not be used (since it requires sleeping and
> I did not investigate sleepable RCU for this purpose).
> 
> So, essentially RCU is not used in DST :)

Works for me!  "Use the right tool for the job!"

quoted text
> Thanks for review, Paul.

							Thanx, Paul