Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> wrote:
> > + fret = ceph_fscrypt_decrypt_pages(inode, &page[pgidx],
> > + off + pgsoff, ext->len);
> > + dout("%s: [%d] 0x%llx~0x%llx fret %d\n", __func__, i,
> > + ext->off, ext->len, fret);
> > + if (fret < 0) {
>
> Possibly, I am missing some logic here. But do we really need to introduce fret?
> Why we cannot user ret here?
>
> > + if (ret == 0)
> > + ret = fret;
> > + break;
> > + }
> > + ret = pgsoff + fret;
Because ret holds the amount of data so far decrypted. We should only return
an error if we didn't decrypt any (ie. ret == 0 at the time of error).
> > +static int ceph_init_request(struct netfs_io_request *rreq, struct file *file)
> > +{
> > + struct ceph_io_request *priv = container_of(rreq, struct ceph_io_request, rreq);
> > + struct inode *inode = rreq->inode;
> > + struct ceph_client *cl = ceph_inode_to_client(inode);
> > + struct ceph_fs_client *fsc = ceph_inode_to_fs_client(inode);
> > + int got = 0, want = CEPH_CAP_FILE_CACHE;
> > + int ret = 0;
> > +
> > + rreq->rsize = 1024 * 1024;
>
> Why do we hardcoded rreq->rsize value?
>
> struct ceph_mount_options {
> unsigned int flags;
>
> unsigned int wsize; /* max write size */
> unsigned int rsize; /* max read size */
> unsigned int rasize; /* max readahead */
> unsigned int congestion_kb; /* max writeback in flight */
> unsigned int caps_wanted_delay_min, caps_wanted_delay_max;
> int caps_max;
> unsigned int max_readdir; /* max readdir result (entries) */
> unsigned int max_readdir_bytes; /* max readdir result (bytes) */
>
> bool new_dev_syntax;
>
> /*
> * everything above this point can be memcmp'd; everything below
> * is handled in compare_mount_options()
> */
>
> char *snapdir_name; /* default ".snap" */
> char *mds_namespace; /* default NULL */
> char *server_path; /* default NULL (means "/") */
> char *fscache_uniq; /* default NULL */
> char *mon_addr;
> struct fscrypt_dummy_policy dummy_enc_policy;
> };
>
> Why we don't use fsc->mount_options->rsize?
Actually, I should get rid of rreq->rsize since there's now a function,
->prepare_read() to deal with this.
> > + loff_t limit = max(i_size_read(inode), fsc->max_file_size);
>
> Do we need to take into account the quota max bytes here?
Could be.
> > +/*
> > + * Size of allocations for default netfs_io_(sub)request object slabs and
> > + * mempools. If a filesystem's request and subrequest objects fit within this
> > + * size, they can use these otherwise they must provide their own.
> > + */
> > +#define NETFS_DEF_IO_REQUEST_SIZE (sizeof(struct netfs_io_request) + 24)
>
> Why do we hardcode 24 here? What's about named constant? And why namely 24?
>
> > +#define NETFS_DEF_IO_SUBREQUEST_SIZE (sizeof(struct netfs_io_subrequest) + 16)
>
> The same question about 16.
See the comment. 24 allows three extra words and 16 two. Actually, I should
probably express it as a multiple of sizeof(long). But this allows netfslib
to allocate (sub)request structs for ceph from the default mempool by allowing
a little bit of extra space.
David
