On Thu, 2019-11-14 at 17:15 +0200, Boaz Harrosh wrote:
> NetApp Security WARNING: This is an external email. Do not click links or open
> attachments unless you recognize the sender and know the content is safe.
>
>
>
>
> 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?
Go for it!
>
> > > +}
>
> Much obliged
> Boaz
