On Thu, Feb 09 2017, Shaohua Li wrote: > On Fri, Feb 10, 2017 at 05:08:54PM +1100, Neil Brown wrote: >> On Tue, Feb 07 2017, Shaohua Li wrote: >> >> > Currently MD is rebusing some bio fields. To remove the hack, we attach >> > extra data to each bio. Each personablity can attach extra data to the >> > bios, so we don't need to rebuse bio fields. >> >> I must say that I don't really like this approach. >> Temporarily modifying ->bi_private and ->bi_end_io seems >> .... intrusive. I suspect it works, but I wonder if it is really >> robust in the long term. >> >> How about a different approach.. Your main concern with my first patch >> was that it called md_write_start() and md_write_end() much more often, >> and these performed atomic ops on "global" variables, particular >> writes_pending. >> >> We could change writes_pending to a per-cpu array which we only count >> occasionally when needed. As writes_pending is updated often and >> checked rarely, a per-cpu array which is summed on demand seems >> appropriate. >> >> The following patch is an early draft - it doesn't obviously fail and >> isn't obviously wrong to me. There is certainly room for improvement >> and may be bugs. >> Next week I'll work on collection the re-factoring into separate >> patches, which are possible good-to-have anyway. > > For your first patch, I don't have much concern. It's ok to me. What I don't > like is the bi_phys_segments handling part. The patches add a lot of logic to > handle the reference count. They should work, but I'd say it's not easy to > understand and could be error prone. What we really need is a reference count > for the bio, so let's just add a reference count. That's my logic and it's > simple. We already have two reference counts, and you want to add a third one. bi_phys_segments is currently used for two related purposes. It counts the number of stripe_heads currently attached to the bio so that when the count reaches zero: 1/ ->writes_pending can be decremented 2/ bio_endio() can be called. When the code was written, the __bi_remaining counter didn't exist. Now it does and it is integrated with bio_endio() so it should make the code easier to understand if we just use bio_endio() rather and doing our own accounting. That just leaves '1'. We can easily decrement ->writes_pending directly instead of decrementing a per-bio refcount, and then when it reaches zero, decrement ->writes_pending. As you pointed out, that comes with a cost. If ->writes_pending is changed to a per-cpu array which is summed on demand, the cost goes away. Having an extra refcount in the bio just adds a level of indirection that doesn't (that I can see) provide actual value. > > For the modifying bi_private and bi_end_io part, I saw some filesystems are > using this way, at least btrfs. If this is really intrusive, is cloning a bio > better? The bio belongs to the filesystem. It allocated it and can do whatever it likes with bi_end_io and bi_private. I don't think a block device driver should ever change bi_private of bi_end_io of a bio that it was passed (if it allocates its own bios, it can of course change those). I don't think cloning the bio would really help, though you could probably make something work. Thanks, NeilBrown
Attachment:
signature.asc
Description: PGP signature
