On 29/10/2019 22:08, Schumaker, Anna wrote:
> Hi Boaz,
>
> On Thu, 2019-09-26 at 05:07 +0300, Boaz Harrosh wrote:
>> zufs Has two ways to do IO.
<>
>> +static int rw_overflow_handler(struct zuf_dispatch_op *zdo, void *arg,
>> + ulong max_bytes)
>> +{
>> + struct zufs_ioc_IO *io = container_of(zdo->hdr, typeof(*io), hdr);
This one is setting the typed pointer @io to be the same of what @zdo->hdr is
>> + struct zufs_ioc_IO *io_user = arg;
>> + int err;
>> +
>> + *io = *io_user;
This one is deep copying the full size structure pointed to by io_user
to the space pointed to by io. (same as zdo->hdr)
Same as memcpy(io, io_user, sizeof(*io))
>
> It looks like you're setting *io using the container_of() macro a few lines above, and then
> overwriting it here without ever using it. Can you remove one of these to make it clearer which
> one you meant to use?
>
These are not redundant its the confusing C thing where declarations
of pointers + assignment means the pointer and not the content.
This code is correct
>> +
>> + err = _ioc_bounds_check(&io->ziom, &io_user->ziom, arg + max_bytes);
>> + if (unlikely(err))
>> + return err;
>> +
>> + if ((io->hdr.err == -EZUFS_RETRY) &&
>> + io->ziom.iom_n && _zufs_iom_pop(io->iom_e)) {
>> +
>> + zuf_dbg_rw(
>> + "[%s]zuf_iom_execute_sync(%d) max=0x%lx iom_e[%d] => %d\n",
>> + zuf_op_name(io->hdr.operation), io->ziom.iom_n,
>> + max_bytes, _zufs_iom_opt_type(io_user->iom_e),
>> + io->hdr.err);
>> +
>> + io->hdr.err = zuf_iom_execute_sync(zdo->sb, zdo->inode,
>> + io_user->iom_e,
>> + io->ziom.iom_n);
>> + return EZUF_RETRY_DONE;
>> + }
<>
>> +static ssize_t _IO_gm(struct zuf_sb_info *sbi, struct inode *inode,
>> + ulong *on_stack, uint max_on_stack,
>> + struct iov_iter *ii, struct kiocb *kiocb,
>> + struct file_ra_state *ra, uint rw)
>> +{
>> + ssize_t size = 0;
>> + ssize_t ret = 0;
>> + enum big_alloc_type bat;
>> + ulong *bns;
>> + uint max_bns = min_t(uint,
>> + md_o2p_up(iov_iter_count(ii) + (kiocb->ki_pos & ~PAGE_MASK)),
>> + ZUS_API_MAP_MAX_PAGES);
>> +
>> + bns = big_alloc(max_bns * sizeof(ulong), max_on_stack, on_stack,
>> + GFP_NOFS, &bat);
>> + if (unlikely(!bns)) {
>> + zuf_err("life was more simple on the stack max_bns=%d\n",
>> + max_bns);
>> + return -ENOMEM;
>> + }
>> +
>> + while (iov_iter_count(ii)) {
>> + ret = _IO_gm_inner(sbi, inode, bns, max_bns, ii, ra,
>> + kiocb->ki_pos, rw);
>> + if (unlikely(ret < 0))
>> + break;
>> +
>> + kiocb->ki_pos += ret;
>> + size += ret;
>> + }
>> +
>> + big_free(bns, bat);
>> +
>> + return size ?: ret;
>
> It looks like you're returning "ret" if the ternary evaluates to false, but it's not clear to
> me what is returned if it evaluates to true. It's possible it's okay, but I just don't know
> enough about how ternaries work in this case.
>
Yes Thanks, Will fix. Not suppose to use this in the Kernel.
>> +}
>> +
<>
>> +int zuf_iom_execute_sync(struct super_block *sb, struct inode *inode,
>> + __u64 *iom_e_user, uint iom_n)
>> +{
>> + struct zuf_sb_info *sbi = SBI(sb);
>> + struct t2_io_state rd_tis = {};
>> + struct t2_io_state wr_tis = {};
>> + struct _iom_exec_info iei = {};
>> + int err, err_r, err_w;
>> +
>> + t2_io_begin(sbi->md, READ, NULL, 0, -1, &rd_tis);
>> + t2_io_begin(sbi->md, WRITE, NULL, 0, -1, &wr_tis);
>> +
>> + iei.sb = sb;
>> + iei.inode = inode;
>> + iei.rd_tis = &rd_tis;
>> + iei.wr_tis = &wr_tis;
>> + iei.iom_e = iom_e_user;
>> + iei.iom_n = iom_n;
>> + iei.print = 0;
>> +
>> + err = _iom_execute_inline(&iei);
>> +
>> + err_r = t2_io_end(&rd_tis, true);
>> + err_w = t2_io_end(&wr_tis, true);
>> +
>> + /* TODO: not sure if OK when _iom_execute return with -ENOMEM
>> + * In such a case, we might be better of skiping t2_io_ends.
>> + */
>> + return err ?: (err_r ?: err_w);
>
> Same question here.
>
> Thanks,
> Anna
>
Yes Will fix
Thanks Anna
Can I put Reviewed-by on this patch?
>> +}
Much obliged
Boaz
