[lkml]   [2008]   [Jun]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH 2 of 3] block: Block layer data integrity support
    On Tue, Jun 17 2008, Martin K. Petersen wrote:
    > >>>>> "Jens" == Jens Axboe <> writes:
    > Jens,
    > I've fixed pretty much everything you pointed out. So unless
    > otherwise noted it's an ACK.

    Great, I'll hold off including the other two patches until a new posting
    of the main patch.

    > > + /* Allocate kernel buffer for protection data */
    > > + len = sectors * blk_integrity_tuple_size(bi);
    > > + buf = kmalloc(len, GFP_NOIO | q->bounce_gfp);
    > > + if (unlikely(buf == NULL)) {
    > > + printk(KERN_ERR "could not allocate integrity buffer\n");
    > > + return -EIO;
    > > + }
    > Jens> Is that good enough, don't you want to handle this error
    > Jens> condition? IOW, doesn't this allocation want mempool backing or
    > Jens> similar?
    > When I originally wrote this I had a couple of mempools that worked
    > well with ext2/3 because they blow everything into 4KB (or 1KB)
    > atoms. Due to the problems with ext2/3 modifying pages in flight I've
    > mostly used XFS and btrfs for development. And they both generate a
    > much more varied set of bio sizes that in turn will require a whole
    > whack of different sized integrity pools.
    > I did gather quite a bit of statistics from runs with different
    > filesystems a few months ago. kmalloc provided a good set of pre-made
    > sizes and I felt it was an overkill to replicate that. But you are
    > right that we should probably be more conservative in terms of failing
    > the I/O. I'll look at it again.

    You are right, a strict mempool solution will not be feasible (or at
    least it will be very wasteful). I guess a temporary solution would be
    to add __GFP_NOFAIL for this allocation.

    > > struct bio_pair {
    > > struct bio bio1, bio2;
    > > struct bio_vec bv1, bv2;
    > > +#if defined(CONFIG_BLK_DEV_INTEGRITY)
    > > + struct bip bip1, bip2;
    > > + struct bio_vec iv1, iv2;
    > > +#endif
    > > atomic_t cnt;
    > > int error;
    > > };
    > Jens> That's somewhat of a shame, it makes bio_pair a LOT bigger. bio
    > Jens> grows a pointer if CONFIG_BLK_DEV_INTEGRITY, that we can live
    > Jens> with. In reality, very few people will use this stuff so adding
    > Jens> a sizable chunk of data to struct bio_pair is somewhat of a
    > Jens> bother.
    > Yeah, well. Wasn't sure what else to do. But the pool is tiny (2
    > entries by default) and only pktdvd and raid 0/10 actually use
    > bio_pairs. I figured if you had CONFIG_BLK_DEV_INTEGRITY on you'd
    > probably want to use integrity it on your MD disks anyway. And on
    > your desktop box with pktdvd integrity wasn't likely to be compiled
    > in.

    I'm not sure there IS a better solution, just noting that it's a bit of
    a shame to grow it that much...

    > Dynamic allocation would defeat the purpose of the pool. But I guess
    > I could make another dedicated bio_integrity_pair pool and wire the
    > integrity portion into bio_pair using pointers. What do you think?

    Doing a quick check, bio_pair is 248 bytes on x86-64 currently. struct
    bio is around 80 bytes or so, bio_vec is 16 bytes. So that's about 200
    extra bytes, making the bio_pair around 440 bytes or so - indeed a
    sizable increase in size. The bio_pair is only used for rare splitting,
    so it's not THAT big of an issue.

    So lets just keep it as-is, I think.

    Jens Axboe

     \ /
      Last update: 2008-06-17 09:23    [W:0.026 / U:59.476 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site