Re: Locking in disk(9) api

[jnemeth%victoria.tc.ca@localhost (John Nemeth)]

On May 22,  3:19am, Michael van Elst wrote:
} On Tue, Dec 29, 2009 at 03:53:58PM -0800, John Nemeth wrote:
} 
} >      In the case in question the real benefit is avoiding a panic when
} > dk_stats->io_busy goes negative.
} 
} The producer side is protected by the corresponding locks in each
} driver (now also in the dm driver). No panic there.

     tls was suggesting that we don't use locks on the producer side
which would lead to corrupt stats.

} > The system would have to be changed
} > to ignore obviously corrupt stats.  At which point, you might as well
} > not bother keeping them since they will be completely unreliable.
} 
} Reading the stats from userland may yield corrupted stats, temporarily.
} 
} In reality however, the 'corruption' only means slightly inconsistent
} data (e.g. old rxfer, new rbytes within a sample). The chance of
} actually seeing a corrupted 64bit number due to non-atomic updates
} is pretty low.

     True, however...

     Reading subr_disk.c shows that the disk_* routines simply call the
equivalent iostat_* routines in subr_iostat.c.  subr_iostat.c has a
lock that protects the addition and deletion of disks to the iostat
chain:

krwlock_t iostatlist_lock;

This lock is used by iostat_alloc() and iostat_free().  It is also used
to protect against changes to the chain while getting the info for the
hw.disknames, hw.iostatnames, and hw.iostats sysctls.  Would it not
make sense to use the same lock to protect the stats themselves from
being updated while they are being read (especially now that sysctl is
used to read them instead of kvm)?

}-- End of excerpt from Michael van Elst