Re: [PATCH 11/16] zuf: Write/Read implementation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux