lkml.org 
[lkml]   [2015]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/6] Refactor code to use new sas_device refcount
    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.
    >


    \
     
     \ /
      Last update: 2015-07-12 06:21    [W:3.087 / U:0.144 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site