Re: [RFC PATCH 33/35] ceph: Use netfslib [INCOMPLETE]

[David Howells]

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