Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756113AbbEVIzf (ORCPT ); Fri, 22 May 2015 04:55:35 -0400 Received: from mail.linux-iscsi.org ([67.23.28.174]:58421 "EHLO linux-iscsi.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751355AbbEVIzc (ORCPT ); Fri, 22 May 2015 04:55:32 -0400 Message-ID: <1432284930.898.19.camel@haakon3.risingtidesystems.com> Subject: Re: [PATCH-v2 1/9] target: Convert se_node_acl->device_list[] to RCU hlist From: "Nicholas A. Bellinger" To: Christoph Hellwig Cc: "Nicholas A. Bellinger" , target-devel , linux-scsi , linux-kernel , Hannes Reinecke , Christoph Hellwig , Sagi Grimberg , "Paul E. McKenney" Date: Fri, 22 May 2015 01:55:30 -0700 In-Reply-To: <20150522082411.GB5384@infradead.org> References: <1432275071-28882-1-git-send-email-nab@daterainc.com> <1432275071-28882-2-git-send-email-nab@daterainc.com> <20150522082411.GB5384@infradead.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.4.4-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4612 Lines: 147 (resending) On Fri, 2015-05-22 at 01:24 -0700, Christoph Hellwig wrote: > > - spin_lock_irqsave(&se_sess->se_node_acl->device_list_lock, flags); > > - se_cmd->se_deve = se_sess->se_node_acl->device_list[unpacked_lun]; > > - if (se_cmd->se_deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS) { > > - struct se_dev_entry *deve = se_cmd->se_deve; > > - > > + rcu_read_lock(); > > + deve = target_nacl_find_deve(nacl, unpacked_lun); > > + if (deve) { > > deve->total_cmds++; > > This update will now be racy, ditto for the read/write_bytes update > later. This should become an atomic_long_t increment, yes..? > > > +bool target_lun_is_rdonly(struct se_cmd *cmd) > > +{ > > + struct se_session *se_sess = cmd->se_sess; > > + struct se_dev_entry *deve; > > + bool ret; > > + > > + if (cmd->se_lun->lun_access & TRANSPORT_LUNFLAGS_READ_ONLY) > > + return true; > > + > > + rcu_read_lock(); > > + deve = target_nacl_find_deve(se_sess->se_node_acl, cmd->orig_fe_lun); > > + ret = (deve && deve->lun_flags & TRANSPORT_LUNFLAGS_READ_ONLY); > > + rcu_read_unlock(); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL(target_lun_is_rdonly); > > This should be a separate prep patch like, like it was in my original > version. I also still think you want this whole patch: > > http://git.infradead.org/users/hch/scsi.git/commitdiff/e9a71bda1a120e0488c5c4e4b2f17f14333e2dc6 > > as storing a pointer to the dev entry without a refcount is bound to > cause trouble. I don't have a tree with all the patches applied > available, but I doubt it fully gets that right. > Yes, this helper is from your patch above. Considering there is a single user of it here, and complexities involved for a RCU conversion + bisect, is it really work adding as a separate patch ahead of this one..? > > +void target_pr_kref_release(struct kref *kref) > > +{ > > + struct se_dev_entry *deve = container_of(kref, struct se_dev_entry, > > + pr_kref); > > + complete(&deve->pr_comp); > > } > > > > /* core_enable_device_list_for_node(): > > > + kref_put(&orig->pr_kref, target_pr_kref_release); > > + wait_for_completion(&orig->pr_comp); > > > > > + kref_put(&orig->pr_kref, target_pr_kref_release); > > /* > > - * Disable struct se_dev_entry LUN ACL mapping > > + * Before fireing off RCU callback, wait for any in process SPEC_I_PT=1 > > + * or REGISTER_AND_MOVE PR operation to complete. > > */ > > + wait_for_completion(&orig->pr_comp); > > + kfree_rcu(orig, rcu_head); > > The release callback should just call kfree_rcu, no need to wait for the > release in the caller. > Why doesn't se_dev_entry release this need to wait for the special case references to drop..? > Also can you drop the _pr from the name? It's a generic refcount now > even if the PR code is the only consumer so far. > > > +void target_pr_kref_release(struct kref *); > > Instead of exporting the release function it would be much more obvious > to have a > > void target_deve_put(struct se_dev_entry *deve) > { > kref_put(&deve->pr_kref, target_deve_release); > } > > helper. Probably paired with one for the get side. > Sure. Adding this now. > > static void core_scsi3_lunacl_undepend_item(struct se_dev_entry *se_deve) > > { > > - struct se_lun_acl *lun_acl = se_deve->se_lun_acl; > > + struct se_lun_acl *lun_acl; > > struct se_node_acl *nacl; > > struct se_portal_group *tpg; > > + > > + if (!se_deve) { > > + pr_err("core_scsi3_lunacl_undepend_item passed NULL se_deve\n"); > > + dump_stack(); > > + return; > > + } > > How could this happen and how is it related to this patch? > Dropped. > > - if (!deve->se_lun || !deve->se_lun_acl) { > > - spin_unlock_irq(&nacl->device_list_lock); > > + rcu_read_lock(); > > + deve = target_nacl_find_deve(nacl, lacl->mapped_lun); > > + if (!deve) { > > + rcu_read_unlock(); > > return -ENODEV; > > So previously a lot of these files returned -ENODEV when not having > an explicit node ACL, and now they don't. If that was intentional > it should be documented in the changelog, or preferably moved into > a preparation patch of its own. > Ok, will update the changelog for this. > > + struct se_node_acl *se_node_acl; > > Where is this field coming from? It's not documented in the changelog > and doesn't seem to be actually used either. > Nice catch. Dropping this unused pointer now. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/