Re: [PATCH 04/12] DRBD: request

[Nikanth K]

On Mon, Mar 23, 2009 at 9:17 PM, Philipp Reisner
<philipp.reisner@xxxxxxxxxx> wrote:
<snip>

> +
> +static inline struct drbd_request *drbd_req_new(struct drbd_conf *mdev,
> + Â Â Â struct bio *bio_src)
> +{
> + Â Â Â struct bio *bio;
> + Â Â Â struct drbd_request *req =
> + Â Â Â Â Â Â Â mempool_alloc(drbd_request_mempool, GFP_NOIO);
> + Â Â Â if (likely(req)) {
> + Â Â Â Â Â Â Â bio = bio_clone(bio_src, GFP_NOIO); /* XXX cannot fail?? */

I think, bio_clone can fail.


<snip>



> +#define OVERLAPS overlaps(sector, size, i->sector, i->size)
> + Â Â Â Â Â Â Â slot = tl_hash_slot(mdev, sector);
> + Â Â Â Â Â Â Â hlist_for_each_entry(i, n, slot, colision) {
> + Â Â Â Â Â Â Â Â Â Â Â if (OVERLAPS) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ALERT("LOGIC BUG: completed: %p %llus +%u; "
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "other: %p %llus +%u\n",
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â req, (unsigned long long)sector, size,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â i, (unsigned long long)i->sector, i->size);
> + Â Â Â Â Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â }
> +
> + Â Â Â Â Â Â Â /* maybe "wake" those conflicting epoch entries
> + Â Â Â Â Â Â Â Â* that wait for this request to finish.
> + Â Â Â Â Â Â Â Â*
> + Â Â Â Â Â Â Â Â* currently, there can be only _one_ such ee
> + Â Â Â Â Â Â Â Â* (well, or some more, which would be pending
> + Â Â Â Â Â Â Â Â* DiscardAck not yet sent by the asender...),
> + Â Â Â Â Â Â Â Â* since we block the receiver thread upon the
> + Â Â Â Â Â Â Â Â* first conflict detection, which will wait on
> + Â Â Â Â Â Â Â Â* misc_wait. Âmaybe we want to assert that?
> + Â Â Â Â Â Â Â Â*
> + Â Â Â Â Â Â Â Â* anyways, if we found one,
> + Â Â Â Â Â Â Â Â* we just have to do a wake_up. Â*/
> +#undef OVERLAPS
> +#define OVERLAPS overlaps(sector, size, e->sector, e->size)

These #defines can be removed? Or Can be parameterized and defined
only once. They are being defined and undefined repeatedly.

> + Â Â Â Â Â Â Â slot = ee_hash_slot(mdev, req->sector);
> + Â Â Â Â Â Â Â hlist_for_each_entry(e, n, slot, colision) {
> + Â Â Â Â Â Â Â Â Â Â Â if (OVERLAPS) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â wake_up(&mdev->misc_wait);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
> + Â Â Â Â Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â }
> + Â Â Â }
> +#undef OVERLAPS
> +}
> +
> +static void _complete_master_bio(struct drbd_conf *mdev,
> + Â Â Â struct drbd_request *req, int error)
<snip>

> +
> +STATIC int drbd_make_request_common(struct drbd_conf *mdev, struct bio *bio)
> +{
> + Â Â Â const int rw = bio_rw(bio);
> + Â Â Â const int size = bio->bi_size;
> + Â Â Â const sector_t sector = bio->bi_sector;
> + Â Â Â struct drbd_barrier *b = NULL;
> + Â Â Â struct drbd_request *req;
> + Â Â Â int local, remote;
> + Â Â Â int err = -EIO;
> +
> + Â Â Â /* allocate outside of all locks; */
> + Â Â Â req = drbd_req_new(mdev, bio);
> + Â Â Â if (!req) {
> + Â Â Â Â Â Â Â dec_ap_bio(mdev);
> + Â Â Â Â Â Â Â /* only pass the error to the upper layers.
> + Â Â Â Â Â Â Â Â* if user cannot handle io errors, thats not our business. */
> + Â Â Â Â Â Â Â ERR("could not kmalloc() req\n");
> + Â Â Â Â Â Â Â bio_endio(bio, -ENOMEM);
> + Â Â Â Â Â Â Â return 0;

Seems to be always returning zero and passing the error through the
bio. So this could be changed to return void.

<snip>

> +
> +int drbd_make_request_26(struct request_queue *q, struct bio *bio)

Always returns zero again.

<snip>

> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â bio_split_pool,
> +#endif

Could be removed for mainline inclusion.


Thanks
Nikanth
N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i