On Tue, Apr 17, 2012 at 10:56 PM, FUJITA Tomonori
<fujita.tomonori@xxxxxxxxxxxxx> wrote:
> On Sun, 15 Apr 2012 13:48:12 +1000
> ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote:
>
>> An updated patch to add UNMAP. Here we just try to unmap the region as
>> it was given to us by the initiator
>> and rely on the initiator to make sure that it only try to unmap data
>> on physical block boundaries.
>>
>> Second is a patch to automatically set
>> LogicalBlocksPerPhysicalBlockExponent anytime we open a backing file.
>> This is used in READCAPACITY16 to tell the initiators the relations
>> between logical blocks and physical blocks
>> on the target, and should guide the initiator to do the right thing
>> when using the UNMAP CDB
>
>>
>> From 8c1c0becbb91e2b2346c7261dbbd6be028f5ff32 Mon Sep 17 00:00:00 2001
>> From: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
>> Date: Sun, 15 Apr 2012 13:41:18 +1000
>> Subject: [PATCH 2/2] SBC: When opening the backing file, check the filesystem blocksize and update LogicalBlocksPerPhysicalBlockExponent accodingly.
>>
>> This is used to tell the initiator what the allignment between LBAs and the underlying storage blocksize is.
>>
>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
>> ---
>> usr/bs_aio.c | 19 +++++++++++++------
>> usr/bs_rdwr.c | 13 +++++++++++--
>> usr/bs_ssc.c | 2 +-
>> usr/util.c | 8 +++++---
>> usr/util.h | 3 ++-
>> 5 files changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/usr/bs_aio.c b/usr/bs_aio.c
>> index cbd91ba..4db5beb 100644
>> --- a/usr/bs_aio.c
>> +++ b/usr/bs_aio.c
>> @@ -304,6 +304,7 @@ static int bs_aio_open(struct scsi_lu *lu, char *path, int *fd, uint64_t *size)
>> {
>> struct bs_aio_info *info = BS_AIO_I(lu);
>> int ret, afd;
>> + uint32_t blksize = 0;
>>
>> info->iodepth = AIO_MAX_IODEPTH;
>> eprintf("create aio context for tgt:%d lun:%"PRId64 ", max iodepth:%d\n",
>> @@ -331,13 +332,13 @@ static int bs_aio_open(struct scsi_lu *lu, char *path, int *fd, uint64_t *size)
>>
>> eprintf("open %s, RW, O_DIRECT for tgt:%d lun:%"PRId64 "\n",
>> path, info->lu->tgt->tid, info->lu->lun);
>> - *fd = backed_file_open(path, O_RDWR|O_LARGEFILE|O_DIRECT, size);
>> + *fd = backed_file_open(path, O_RDWR|O_LARGEFILE|O_DIRECT, size, &blksize);
>> /* If we get access denied, try opening the file in readonly mode */
>> if (*fd == -1 && (errno == EACCES || errno == EROFS)) {
>> eprintf("open %s, READONLY, O_DIRECT for tgt:%d lun:%"PRId64 "\n",
>> path, info->lu->tgt->tid, info->lu->lun);
>> *fd = backed_file_open(path, O_RDONLY|O_LARGEFILE|O_DIRECT,
>> - size);
>> + size, &blksize);
>> lu->attrs.readonly = 1;
>> }
>> if (*fd < 0) {
>> @@ -346,12 +347,18 @@ static int bs_aio_open(struct scsi_lu *lu, char *path, int *fd, uint64_t *size)
>> ret = *fd;
>> goto remove_tgt_evt;
>> }
>> - else {
>> - eprintf("%s opened successfully for tgt:%d lun:%"PRId64 "\n",
>> - path, info->lu->tgt->tid, info->lu->lun);
>> - return 0;
>> +
>> + eprintf("%s opened successfully for tgt:%d lun:%"PRId64 "\n",
>> + path, info->lu->tgt->tid, info->lu->lun);
>> +
>> + lu->attrs.lbppbe = 0;
>
> lbppbe should be already zero'ed.
It is only zero the first time it is called for the LUN.
A user could make the lun offline and then online it again (with a
different backing file, on a different filesystem, with a different
blocksize)
>
>> + while (blksize > (1U << lu->blk_shift)) {
>> + lu->attrs.lbppbe++;
>> + blksize >>= 1;
>> }
>
> I think that we should not set lbppbe blindly. Only when users don't
> specify lbppbe via tgtadm.
Ill send a new patch so that we set the LBPPBE automatically, unless
the user has overridden the setting by providing it explicitely.
If the user has used tgtadm to set the LBPPBE explicitely, we leave
that value as is and do not update it if the file is offline/online
again.
>
> Also can we have a helper function to avoid this duplicated code?
Yes, will do.
>
>
>
>> + return 0;
>> +
>> remove_tgt_evt:
>> tgt_event_del(afd);
>> close_eventfd:
>> diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c
>> index 9e15603..a917fab 100644
>> --- a/usr/bs_rdwr.c
>> +++ b/usr/bs_rdwr.c
>> @@ -231,16 +231,25 @@ static void bs_rdwr_request(struct scsi_cmd *cmd)
>>
>> static int bs_rdwr_open(struct scsi_lu *lu, char *path, int *fd, uint64_t *size)
>> {
>> - *fd = backed_file_open(path, O_RDWR|O_LARGEFILE|lu->bsoflags, size);
>> + uint32_t blksize = 0;
>> +
>> + *fd = backed_file_open(path, O_RDWR|O_LARGEFILE|lu->bsoflags, size,
>> + &blksize);
>> /* If we get access denied, try opening the file in readonly mode */
>> if (*fd == -1 && (errno == EACCES || errno == EROFS)) {
>> *fd = backed_file_open(path, O_RDONLY|O_LARGEFILE|lu->bsoflags,
>> - size);
>> + size, &blksize);
>> lu->attrs.readonly = 1;
>> }
>> if (*fd < 0)
>> return *fd;
>>
>> + lu->attrs.lbppbe = 0;
>> + while (blksize > (1U << lu->blk_shift)) {
>> + lu->attrs.lbppbe++;
>> + blksize >>= 1;
>> + }
>> +
>> return 0;
>> }
>>
>> diff --git a/usr/bs_ssc.c b/usr/bs_ssc.c
>> index 6412972..93d08a6 100644
>> --- a/usr/bs_ssc.c
>> +++ b/usr/bs_ssc.c
>> @@ -619,7 +619,7 @@ static int bs_ssc_open(struct scsi_lu *lu, char *path, int *fd, uint64_t *size)
>>
>> ssc = dtype_priv(lu);
>>
>> - *fd = backed_file_open(path, O_RDWR | O_LARGEFILE, size);
>> + *fd = backed_file_open(path, O_RDWR | O_LARGEFILE, size, NULL);
>> if (*fd < 0) {
>> eprintf("Could not open %s %m\n", path);
>> return *fd;
>> diff --git a/usr/util.c b/usr/util.c
>> index c78a999..4cf8291 100644
>> --- a/usr/util.c
>> +++ b/usr/util.c
>> @@ -82,7 +82,7 @@ int chrdev_open(char *modname, char *devpath, uint8_t minor, int *fd)
>> return 0;
>> }
>>
>> -int backed_file_open(char *path, int oflag, uint64_t *size)
>> +int backed_file_open(char *path, int oflag, uint64_t *size, uint32_t *blksize)
>> {
>> int fd, err;
>> struct stat64 st;
>> @@ -99,9 +99,11 @@ int backed_file_open(char *path, int oflag, uint64_t *size)
>> goto close_fd;
>> }
>>
>> - if (S_ISREG(st.st_mode))
>> + if (S_ISREG(st.st_mode)) {
>> *size = st.st_size;
>> - else if (S_ISBLK(st.st_mode)) {
>> + if (blksize)
>> + *blksize = st.st_blksize;
>> + } else if (S_ISBLK(st.st_mode)) {
>> err = ioctl(fd, BLKGETSIZE64, size);
>> if (err < 0) {
>> eprintf("Cannot get size, %m\n");
>> diff --git a/usr/util.h b/usr/util.h
>> index 4bf157d..c016e5a 100644
>> --- a/usr/util.h
>> +++ b/usr/util.h
>> @@ -62,7 +62,8 @@
>>
>> extern int get_blk_shift(unsigned int size);
>> extern int chrdev_open(char *modname, char *devpath, uint8_t minor, int *fd);
>> -extern int backed_file_open(char *path, int oflag, uint64_t *size);
>> +extern int backed_file_open(char *path, int oflag, uint64_t *size,
>> + uint32_t *blksize);
>> extern int set_non_blocking(int fd);
>> extern int str_to_open_flags(char *buf);
>> extern char *open_flags_to_str(char *dest, int flags);
>> --
>> 1.7.3.1
>>
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
