lkml.org 
[lkml]   [2008]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 4 of 7] block: bio data integrity support
    Date
    "Martin K. Petersen" <martin.petersen@oracle.com> writes:

    Comments inlined below.

    > +struct bip *bio_integrity_alloc_bioset(struct bio *bio, gfp_t gfp_mask, unsigned int nr_vecs, struct bio_set *bs)
    > +{
    > + struct bip *bip;
    > + struct bio_vec *bv;
    > + unsigned long idx;
    > +
    > + BUG_ON(bio == NULL);
    > +
    > + bip = mempool_alloc(bs->bio_integrity_pool, gfp_mask);
    > + if (unlikely(bip == NULL)) {
    > + printk(KERN_ERR "%s: could not alloc bip\n", __func__);
    > + return NULL;
    > + }
    > +
    > + memset(bip, 0, sizeof(*bip));
    > + idx = 0;

    That assignment isn't necessary.

    > +int bio_integrity_set_tag(struct bio *bio, void *tag_buf, unsigned int len)
    > +{
    > + struct bip *bip = bio->bi_integrity;
    > + struct blk_integrity *bi = bdev_get_integrity(bio->bi_bdev);
    > + unsigned int nr_sectors;
    > +
    > + BUG_ON(bip->bip_buf == NULL);
    > + BUG_ON(bio_data_dir(bio) != WRITE);
    > +
    > + if (bi->tag_size == 0)
    > + return -1;
    > +
    > + nr_sectors = len / bi->tag_size;
    > +
    > + if (len % 2)
    > + nr_sectors++;

    I see you've changed this to:

    nr_sectors = (len + bi->tag_size - 1) / bi->tag_size;

    why not simply use DIV_ROUND_UP?

    > +
    > + if (bi->sector_size == 4096)
    > + nr_sectors >>= 3;
    > +
    > + if (nr_sectors * bi->tuple_size > bip->bip_size) {
    > + printk(KERN_ERR "%s: tag too big for bio: %u > %u\n",
    > + __func__, nr_sectors * bi->tuple_size, bip->bip_size);
    > + return -1;
    > + }
    > +
    > + bi->set_tag_fn(bip->bip_buf, tag_buf, nr_sectors);
    > +
    > + return 0;
    > +}
    > +EXPORT_SYMBOL(bio_integrity_set_tag);
    > +
    > +/**
    > + * bio_integrity_get_tag - Retrieve a tag buffer from a bio
    > + * @bio: bio to retrieve buffer from
    > + * @tag_buf: Pointer to a buffer for the tag data
    > + * @len: Length of the target buffer
    > + *
    > + * Description: Use this function to retrieve the tag buffer from a
    > + * completed I/O. The size of the integrity buffer must be <= to the
    > + * size reported by bio_integrity_tag_size().
    > + */
    > +int bio_integrity_get_tag(struct bio *bio, void *tag_buf, unsigned int len)
    > +{
    > + struct bip *bip = bio->bi_integrity;
    > + struct blk_integrity *bi = bdev_get_integrity(bio->bi_bdev);
    > + unsigned int nr_sectors;
    > +
    > + BUG_ON(bip->bip_buf == NULL);
    > + BUG_ON(bio_data_dir(bio) != READ);
    > +
    > + if (bi->tag_size == 0)
    > + return -1;
    > +
    > + nr_sectors = len / bi->tag_size;
    > +
    > + if (len % 2)
    > + nr_sectors++;
    > +
    > + if (bi->sector_size == 4096)
    > + nr_sectors >>= 3;
    > +
    > + if (nr_sectors * bi->tuple_size > bip->bip_size) {
    > + printk(KERN_ERR "%s: tag too big for bio: %u > %u\n",
    > + __func__, nr_sectors * bi->tuple_size, bip->bip_size);
    > + return -1;
    > + }
    > +
    > + bi->get_tag_fn(bip->bip_buf, tag_buf, nr_sectors);
    > +
    > + return 0;
    > +}
    > +EXPORT_SYMBOL(bio_integrity_get_tag);

    set_tag and get_tag are almost identical. Any chance you want to factor
    out that code?

    > +/**
    > + * bio_integrity_generate - Generate integrity metadata for a bio
    > + * @bio: bio to generate integrity metadata for
    > + *
    > + * Description: Generates integrity metadata for a bio by calling the
    > + * block device's generation callback function. The bio must have a
    > + * bip attached with enough room to accomodate the generated integrity
    ^^^^^^^^^^
    accommodate

    > + * metadata.
    > + */
    > +static void bio_integrity_generate(struct bio *bio)
    > +{
    > + struct blk_integrity *bi = bdev_get_integrity(bio->bi_bdev);

    Hmm, up until this point you use bi to mean bio_integrity, but now
    it means blk_integrity. Confusion will ensue. ;)

    > + struct blk_integrity_exchg bix;

    struct blk_integrity_exchg is not yet defined in your patch set, so this
    will likely break git bisect.

    > +int bio_integrity_prep(struct bio *bio)
    > +{
    ...
    > + buf = kzalloc(len, GFP_NOIO | q->bounce_gfp);

    Does this actually need to be zeroed?

    > +void bio_integrity_advance(struct bio *bio, unsigned int bytes_done)
    ...
    > + bip_for_each_vec(iv, bip, i) {
    > + if (skip == 0) {
    > + bip->bip_idx = i;
    > + return;
    > + } else if (skip >= iv->bv_len) {
    > + skip -= iv->bv_len;
    > + } else { /* skip < iv->bv_len) */
    > + iv->bv_offset += skip;
    > + iv->bv_len -= skip;
    > + bip->bip_idx = i;
    > + return;
    > + }
    > + }
    > +}

    > +void bio_integrity_trim(struct bio *bio, unsigned int offset, unsigned int sectors)
    > +{
    ...
    > + /* Mark head */
    > + bip_for_each_vec(iv, bip, i) {
    > + if (skip == 0) {
    > + bip->bip_idx = i;
    > + break;
    > + } else if (skip >= iv->bv_len) {
    > + skip -= iv->bv_len;
    > + } else { /* skip < iv->bv_len) */
    > + iv->bv_offset += skip;
    > + iv->bv_len -= skip;
    > + bip->bip_idx = i;
    > + break;
    > + }
    > + }

    The above two loops look pretty much the same to me. Can you factor
    that out to a helper?

    Cheers,

    Jeff


    \
     
     \ /
      Last update: 2008-06-10 22:57    [W:0.034 / U:180.156 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site