Re: [Iscsitarget-devel] [PATCH v2 2/3] user definable SCSI levelblocksize

[Ross S. W. W. <RW...@me...>]

On Feb 28, 2010, at 12:56 PM, "Ross S. W. Walker"  
<RW...@me...> wrote:

> Lars Ellenberg [mailto:lar...@li...] wrote:
>
> Just small cosmetic suggestion.
>
> > @@ -350,12 +353,34 @@ blockio_attach(struct iet_volume
> > *volume, char *args)
> >
> >       volume->private = bio_data;
> >
> > -     if ((err = parse_blockio_params(volume, args)) < 0) {
> > +     err = parse_blockio_params(volume, args);
> > +     if (!err) {
> > +             /* Default to the lower level device logical block  
> size.
> > +              * If the user set an explicit block size,
> > +              * make sure it is compatible. */
> > +             struct request_queue *q = bdev_get_queue(bio_data- 
> >bdev);
> > +             unsigned bsz = queue_logical_block_size(q);
> > +             unsigned bshift = ilog2(bsz);
> > +             if (!volume->blk_shift)
> > +                     volume->blk_shift = bshift;
> > +             else if (volume->blk_shift < bshift) {
> > +                     eprintk("Target %s, LUN %u: "
> > +                             "blocksize (%u) < logical block size  
> of %s (%u). "
> > +                             "Try Type=fileio?\n",
> > +                             volume->target->name, volume->lun,
> > +                             1 << volume->blk_shift,
> > +                             bio_data->path, bsz);
> > +                     err = -EINVAL;
> > +             }
> > +     }
> > +     if (err < 0) {
> >               eprintk("Error attaching Lun %u to Target %s \n",
> >                       volume->lun, volume->target->name);
> >               goto out;
> >       }
> >
> > +     volume->blk_cnt = bio_data->bdev->bd_inode->i_size >> volume- 
> >blk_shift;
> > +
> >       /* Assign a vendor id, generate scsi id if none exists */
> >       gen_scsiid(volume, bio_data->bdev->bd_inode);
> >
> > @@ -363,9 +388,6 @@ blockio_attach(struct iet_volume *volume,
> > char *args)
> >       ClearLURCache(volume);
> >       ClearLUWCache(volume);
> >
> > -     volume->blk_shift = SECTOR_SIZE_BITS;
> > -     volume->blk_cnt = bio_data->bdev->bd_inode->i_size >> volume- 
> >blk_shift;
> > -
> >    out:
> >       if (err < 0)
> >               blockio_detach(volume);
>
> What if we simplify this by setting the default shift
> back from volume.c to here again, and then make the
> whole test/error shorter like so:
>
> -       volume->blk_shift = SECTOR_SIZE_BITS;
> +       /* see Documentation/ABI/testing/sysfs-block */
> +       blk_sz = bdev_logical_block_size(bio_data->bdev);
> +       if (!volume->blk_shift)
> +               volume->blk_shift = ilog2(blk_sz);
> +       else if (volume->blk_shift < ilog2(blk_sz)) {
> +               eprintk("User specified block size (%u) smaller then  
> device %s logical block size (%u)\n",
> +                       (1 << volume->blk_shift), bio_data->path,  
> blk_sz);
> +               err = -EINVAL;
> +               goto out;
> +       }
> +
>
> And in file-io just test and set default shift right
> where we previously set it.
>
> -       lu->blk_shift = SECTOR_SIZE_BITS;
> +       if (!lu->blk_shift)
> +               lu->blk_shift = 9;
> +
>
> And just kill setting the default in volume.c
>
> > +     /* Assume 512 byte block size by default.
> > +      * Do this after ->attach(), so those routines
> > +      * can still distinguish user-supplied vs "default"
> > +      * by checking blk_shift == 0. */
> > +     if (!volume->blk_shift)
> > +             volume->blk_shift = 9;
> > +
>
> It's not a show stopper though and I'd sign-off on the
> existing code, but I think setting the default in
> volume.c and then test/set in blockio would cause
> unneccessary? errors in the log.
>

Actually with the error-out code now, setting the default in volume.c  
will prevent block-io from working on devices with logical block sizes  
larger then the default, so it will need fixing after all, if just to  
remove setting the default in volume.c and add it to file-io/null-io.

-Ross
  
  
______________________________________________________________________
This e-mail, and any attachments thereto, is intended only for use by
the addressee(s) named herein and may contain legally privileged
and/or confidential information. If you are not the intended recipient
of this e-mail, you are hereby notified that any dissemination,
distribution or copying of this e-mail, and any attachments thereto,
is strictly prohibited. If you have received this e-mail in error,
please immediately notify the sender and permanently delete the
original and any copy or printout thereof.