(Removed Cc invalid emails to Nicholas and Andrzej)
Hi,
On Fri, Dec 20, 2024, Homura Akemi wrote:
> Hi Thinh,
>
> On 2024-12-11 0:31 UTC, Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> wrote:
> > 1) Fix Data Corruption
> > ----------------------
> >
> > Properly increment the "len" base on the command requested length instead of
> > the SG entry length.
> >
> > If you're using File backend, then you need to fix target_core_file. If you're
> > using other backend such as Ramdisk, then you need a similar fix there.
>
> I am trying to do some basic rw aging test with this serie on my gadget board.
> When it comes to target_core_iblock, the rw code is less similar to the one in
> target_core_file or ramdisk.
> Could you just spend some extra time explaining what cause the Data
> Corruption issue and how can this fix it ? So that I could perform
> similar fix in
> target_core_iblock on my own, so a full test could start soonner.
>
When we prepare a new generic command for read/write, we call to
target_alloc_sgl(). This will allocate PAGE_SIZE SG entries enough to
handle the se_cmd read/write base on its length. The total length of all
the SG entries combine will be at least se_cmd->data_length.
The typical block size is 512 byte. A page size is typically 4KB. So,
the se_cmd->data_length may not be a multiple of PAGE_SiZE. In
target_core_file, it execute_rw() with this logic:
for_each_sg(sgl, sg, sgl_nents, i) {
bvec_set_page(&aio_cmd->bvecs[i], sg_page(sg), sg->length,
sg->offset);
len += sg->length;
}
// It sets the length to be the iter to be total length of the
// allocated SG entries and not the requested command length:
iov_iter_bvec(&iter, is_write, aio_cmd->bvecs, sgl_nents, len);
aio_cmd->cmd = cmd;
aio_cmd->len = len;
aio_cmd->iocb.ki_pos = cmd->t_task_lba * dev->dev_attrib.block_size;
aio_cmd->iocb.ki_filp = file;
aio_cmd->iocb.ki_complete = cmd_rw_aio_complete;
aio_cmd->iocb.ki_flags = IOCB_DIRECT;
if (is_write && (cmd->se_cmd_flags & SCF_FUA))
aio_cmd->iocb.ki_flags |= IOCB_DSYNC;
// So when we do f_op read/write, we may do more than needed and may
// write bogus data from the extra SG entry length.
if (is_write)
ret = file->f_op->write_iter(&aio_cmd->iocb, &iter);
else
ret = file->f_op->read_iter(&aio_cmd->iocb, &iter);
I did not review target_core_iblock. It may or may not do things
properly.
BR,
Thinh
