Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751831AbbGLEQD (ORCPT ); Sun, 12 Jul 2015 00:16:03 -0400 Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:43111 "EHLO mx0b-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751373AbbGLEQA (ORCPT ); Sun, 12 Jul 2015 00:16:00 -0400 Date: Sat, 11 Jul 2015 21:15:29 -0700 From: Calvin Owens To: Christoph Hellwig CC: Nagalakshmi Nandigama , Praveen Krishnamoorthy , Sreekanth Reddy , Abhijit Mahajan , , , , Subject: Re: [PATCH 2/6] Refactor code to use new sas_device refcount Message-ID: <20150712041529.GA4066426@mail.thefacebook.com> References: <1431661322-3097935-1-git-send-email-calvinowens@fb.com> <1433821856-2815280-1-git-send-email-calvinowens@fb.com> <1433821856-2815280-3-git-send-email-calvinowens@fb.com> <20150703153803.GB4472@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: <20150703153803.GB4472@infradead.org> User-Agent: Mutt/1.5.20 (2009-12-10) X-Originating-IP: [192.168.52.123] X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.14.151,1.0.33,0.0.0000 definitions=2015-07-12_04:2015-07-11,2015-07-12,1970-01-01 signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5822 Lines: 151 On Friday 07/03 at 08:38 -0700, Christoph Hellwig wrote: > > > > +struct _sas_device * > > +mpt2sas_scsih_sas_device_get_by_sas_address_nolock(struct MPT2SAS_ADAPTER *ioc, > > + u64 sas_address) > > Any chance to use a shorter name for this function? E.g. > __mpt2sas_get_sdev_by_addr ? Will do. > > +{ > > + struct _sas_device *sas_device; > > + > > + BUG_ON(!spin_is_locked(&ioc->sas_device_lock)); > > This will blow on UP builds. Please use assert_spin_locked or > lockdep_assert_held instead. And don't ask me which of the two, > that's a mystery I don't understand myself either. Will do. > > struct _sas_device * > > -mpt2sas_scsih_sas_device_find_by_sas_address(struct MPT2SAS_ADAPTER *ioc, > > +mpt2sas_scsih_sas_device_get_by_sas_address(struct MPT2SAS_ADAPTER *ioc, > > u64 sas_address) > > { > > > +static struct _sas_device * > > +_scsih_sas_device_get_by_handle_nolock(struct MPT2SAS_ADAPTER *ioc, u16 handle) > > > static struct _sas_device * > > -_scsih_sas_device_find_by_handle(struct MPT2SAS_ADAPTER *ioc, u16 handle) > > +_scsih_sas_device_get_by_handle(struct MPT2SAS_ADAPTER *ioc, u16 handle) > > Same comments about the function names as above. > > > + struct _sas_device *sas_device; > > + > > + BUG_ON(!spin_is_locked(&ioc->sas_device_lock)); > > Same comment about the right assert helpers as above. > > > @@ -594,9 +634,15 @@ _scsih_sas_device_remove(struct MPT2SAS_ADAPTER *ioc, > > if (!sas_device) > > return; > > > > + /* > > + * The lock serializes access to the list, but we still need to verify > > + * that nobody removed the entry while we were waiting on the lock. > > + */ > > spin_lock_irqsave(&ioc->sas_device_lock, flags); > > - list_del(&sas_device->list); > > - kfree(sas_device); > > + if (!list_empty(&sas_device->list)) { > > + list_del_init(&sas_device->list); > > + sas_device_put(sas_device); > > + } > > spin_unlock_irqrestore(&ioc->sas_device_lock, flags); > > This looks odd to me. Normally you'd have the lock from the list > iteration that finds the device. From looking at the code it seems > like this only called from probe failure paths, though. It seems like > for this case the device simplify shouldn't be added until the probe > succeeds and this function should go away? There's a horrible maze of dependencies on things being on the lists while being added that make this impossible: I spent some time trying to get this to work, but I always end up with no drives. :( (The path through _scsih_probe_sas() seems not to care) I was hopeful your suggestion below about putting the sas_device pointer in ->hostdata would eliminate the need for all the find_by_X() lookups, but some won't go away. > > @@ -1208,12 +1256,15 @@ _scsih_change_queue_depth(struct scsi_device *sdev, int qdepth) > > goto not_sata; > > if ((sas_target_priv_data->flags & MPT_TARGET_FLAGS_VOLUME)) > > goto not_sata; > > + > > spin_lock_irqsave(&ioc->sas_device_lock, flags); > > - sas_device = mpt2sas_scsih_sas_device_find_by_sas_address(ioc, > > + sas_device = mpt2sas_scsih_sas_device_get_by_sas_address_nolock(ioc, > > sas_device_priv_data->sas_target->sas_address); > > - if (sas_device && sas_device->device_info & > > - MPI2_SAS_DEVICE_INFO_SATA_DEVICE) > > + if (sas_device && sas_device->device_info > > + & MPI2_SAS_DEVICE_INFO_SATA_DEVICE) { > > max_depth = MPT2SAS_SATA_QUEUE_DEPTH; > > + sas_device_put(sas_device); > > + } > > Please store a pointer to the sas_device in struct scsi_target ->hostdata > in _scsih_target_alloc and avoid the need for this and other runtime > lookups where we have a scsi_device or scsi_target structure available. Will do. > > @@ -1324,13 +1377,15 @@ _scsih_target_destroy(struct scsi_target *starget) > > > > spin_lock_irqsave(&ioc->sas_device_lock, flags); > > rphy = dev_to_rphy(starget->dev.parent); > > - sas_device = mpt2sas_scsih_sas_device_find_by_sas_address(ioc, > > + sas_device = mpt2sas_scsih_sas_device_get_by_sas_address_nolock(ioc, > > rphy->identify.sas_address); > > if (sas_device && (sas_device->starget == starget) && > > (sas_device->id == starget->id) && > > (sas_device->channel == starget->channel)) > > sas_device->starget = NULL; > > > > + if (sas_device) > > + sas_device_put(sas_device); > > spin_unlock_irqrestore(&ioc->sas_device_lock, flags); > > .. like this one. > > > out: > > @@ -1386,7 +1441,7 @@ _scsih_slave_alloc(struct scsi_device *sdev) > > > > if (!(sas_target_priv_data->flags & MPT_TARGET_FLAGS_VOLUME)) { > > spin_lock_irqsave(&ioc->sas_device_lock, flags); > > - sas_device = mpt2sas_scsih_sas_device_find_by_sas_address(ioc, > > + sas_device = mpt2sas_scsih_sas_device_get_by_sas_address_nolock(ioc, > > sas_target_priv_data->sas_address); > > if (sas_device && (sas_device->starget == NULL)) { > > sdev_printk(KERN_INFO, sdev, > > .. or this one .. > > > @@ -1428,10 +1487,13 @@ _scsih_slave_destroy(struct scsi_device *sdev) > > > > if (!(sas_target_priv_data->flags & MPT_TARGET_FLAGS_VOLUME)) { > > spin_lock_irqsave(&ioc->sas_device_lock, flags); > > - sas_device = mpt2sas_scsih_sas_device_find_by_sas_address(ioc, > > + sas_device = mpt2sas_scsih_sas_device_get_by_sas_address_nolock(ioc, > > sas_target_priv_data->sas_address); > > if (sas_device && !sas_target_priv_data->num_luns) > > sas_device->starget = NULL; > > + > > + if (sas_device) > > + sas_device_put(sas_device); > > spin_unlock_irqrestore(&ioc->sas_device_lock, flags); > > .. and this, and many more. > -- 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/