Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754422AbYFKEGW (ORCPT ); Wed, 11 Jun 2008 00:06:22 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750717AbYFKEGM (ORCPT ); Wed, 11 Jun 2008 00:06:12 -0400 Received: from rgminet01.oracle.com ([148.87.113.118]:59354 "EHLO rgminet01.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750700AbYFKEGM (ORCPT ); Wed, 11 Jun 2008 00:06:12 -0400 To: Jeff Moyer Cc: "Martin K. Petersen" , linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org Subject: Re: [PATCH 4 of 7] block: bio data integrity support From: "Martin K. Petersen" Organization: Oracle References: Date: Wed, 11 Jun 2008 00:05:29 -0400 In-Reply-To: (Jeff Moyer's message of "Tue\, 10 Jun 2008 16\:52\:20 -0400") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Brightmail-Tracker: AAAAAQAAAAI= X-Brightmail-Tracker: AAAAAQAAAAI= X-Whitelist: TRUE X-Whitelist: TRUE Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1784 Lines: 63 >>>>> "Jeff" == Jeff Moyer writes: >> + memset(bip, 0, sizeof(*bip)); >> + idx = 0; Jeff> That assignment isn't necessary. Zap! Jeff> nr_sectors = (len + bi->tag_size - 1) / bi->tag_size; Jeff> why not simply use DIV_ROUND_UP? Fixed. Jeff> set_tag and get_tag are almost identical. Any chance you want Jeff> to factor out that code? Done. Jeff> Hmm, up until this point you use bi to mean bio_integrity, but Jeff> now it means blk_integrity. Confusion will ensue. ;) Err, uhm. There is no bio_integrity. There's the bio integrity payload which I always refer to as struct bip *bip. And struct blk_integrity which is always bi. I'm also anal about using bv for the data bio_vec and iv for the integrity bio_vec. I can't see any place where I'm inconsistent. Jeff> struct blk_integrity_exchg is not yet defined in your patch set, Jeff> so this will likely break git bisect. bio-integrity.patch and blk-integrity.patch are artificially split up to ease the review process. They are not meant to be separate changesets. >> + buf = kzalloc(len, GFP_NOIO | q->bounce_gfp); Jeff> Does this actually need to be zeroed? Nope. >> +void bio_integrity_advance(struct bio *bio, unsigned int > +void bio_integrity_trim(struct bio *bio, unsigned int offset, unsigned int sectors) Jeff> The above two loops look pretty much the same to me. Can you Jeff> factor that out to a helper? I've created helpers for marking head and tail of the ivec. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/