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

["Schumaker, Anna" <Anna.Schumaker@netapp.com>]

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