Skip to contentSkip to conversation

SQUASHME: BUGs squashing in new pnfs for LD code

PATCH In review Boaz Harrosh 2 participants 2 messages May 22, 2011 – May 23, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 SQUASHME: BUGs squashing in new pnfs for LD code

1. In nfs4_write_done_cb: data->write_done_cb comes with a NULL. Just as a guess I call nfs4_write_done_cb() just above it it looked like the right thing to do. With that in, I'm able to write things to file When converting pnfs.c:258 to a WARN_ON.

Benny we might want to set data->write_done_cb somewhere in the none-rpc path? where is it best to do that?
1.5
Same as above for nfs4_read_done.

2. In pnfs_ld_write_done We don't need:

	put_lseg(data->lseg);
	data->lseg = NULL;

It is called in nfs_writedata_release()
2.5 Same for pnfs_ld_read_done

3. In pnfs_ld_write_done:
data->mds_ops->rpc_call_done(NULL, data); crashes with a NULL task. Just pass it with &data->task

Which calls for a cleanup. There is bunch of functions with [task, write_data] API. And the task is always write_data->task

3.5
Same for pnfs_ld_read_done

4. It is now with current code not possible to run with out a *.pg_test* opt. Without, it will crash, reference leak and only do pnfs-IO on a single page. I'll send a patch to check for it in... where the ld->opt is checked.

So define one for objlayout. Which always returns true.

With this I'm able to read/write with pnfs-IO. (lightly test) (With out any WARN_ONs)

One problem I've seen is that I only get up to 16 pages maximum in a single request. I would like to see the 512 pages we had before for a full request. Where should I fix that?

---
 fs/nfs/nfs4proc.c            |    5 +++--
 fs/nfs/objlayout/objio_osd.c |   15 +++++++++++++++
 fs/nfs/pnfs.c                |   14 +++++++-------
 3 files changed, 25 insertions(+), 9 deletions(-)
fs/nfs/nfs4proc.c +3 -2
index 759523a..4bf7c0c 100644
@@ -3205,7 +3205,7 @@ static int nfs4_read_done(struct rpc_task *task, struct nfs_read_data *data)
 	if (!nfs4_sequence_done(task, &data->res.seq_res))
 		return -EAGAIN;
 
-	return data->read_done_cb(task, data);
+	return data->read_done_cb ? data->read_done_cb(task, data) : nfs4_read_done_cb(task, data);
 }
 
 static void nfs4_proc_read_setup(struct nfs_read_data *data, struct rpc_message *msg)
@@ -3250,7 +3250,8 @@ static int nfs4_write_done(struct rpc_task *task, struct nfs_write_data *data)
 {
 	if (!nfs4_sequence_done(task, &data->res.seq_res))
 		return -EAGAIN;
-	return data->write_done_cb(task, data);
+	return data->write_done_cb ? data->write_done_cb(task, data) :
+		nfs4_write_done_cb(task, data);
 }
 
 /* Reset the the nfs_write_data to send the write to the MDS. */
fs/nfs/objlayout/objio_osd.c +15 -0
index 7e46d2b..105d8a6 100644
@@ -989,6 +989,19 @@ ssize_t objio_write_pagelist(struct objlayout_io_state *ol_state, bool stable)
 	return _write_exec(ios);
 }
 
+/*
+ * objlayout_pg_test(). Called by nfs_can_coalesce_requests()
+ *
+ * return 1 :  coalesce page
+ * return 0 :  don't coalesce page
+ */
+int
+objlayout_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev,
+		   struct nfs_page *req)
+{
+	return 1;
+}
+
 static struct pnfs_layoutdriver_type objlayout_type = {
 	.id = LAYOUT_OSD2_OBJECTS,
 	.name = "LAYOUT_OSD2_OBJECTS",
@@ -1008,6 +1021,8 @@ static struct pnfs_layoutdriver_type objlayout_type = {
 
 	.encode_layoutcommit	 = objlayout_encode_layoutcommit,
 	.encode_layoutreturn     = objlayout_encode_layoutreturn,
+
+	.pg_test		= objlayout_pg_test,
 };
 
 void *objio_init_mt(void)
fs/nfs/pnfs.c +7 -7
index 8d2baf8..ea30a52 100644
@@ -256,7 +256,7 @@ put_lseg_common(struct pnfs_layout_segment *lseg)
 {
 	struct inode *inode = lseg->pls_layout->plh_inode;
 
-	BUG_ON(test_bit(NFS_LSEG_VALID, &lseg->pls_flags));
+ 	WARN_ON(test_bit(NFS_LSEG_VALID, &lseg->pls_flags));
 	list_del_init(&lseg->pls_list);
 	if (list_empty(&lseg->pls_layout->plh_segs)) {
 		set_bit(NFS_LAYOUT_DESTROYED, &lseg->pls_layout->plh_flags);
@@ -1125,15 +1125,15 @@ pnfs_ld_write_done(struct nfs_write_data *data)
 {
 	int status;
 
-	put_lseg(data->lseg);
-	data->lseg = NULL;
 	if (!data->pnfs_error) {
 		pnfs_set_layoutcommit(data);
-		data->mds_ops->rpc_call_done(NULL, data);
+		data->mds_ops->rpc_call_done(&data->task, data);
 		data->mds_ops->rpc_release(data);
 		return 0;
 	}
 
+	put_lseg(data->lseg);
+	data->lseg = NULL;
 	dprintk("%s: pnfs_error=%d, retry via MDS\n", __func__,
 		data->pnfs_error);
 	status = nfs_initiate_write(data, NFS_CLIENT(data->inode), data->mds_ops, NFS_FILE_SYNC);
@@ -1173,15 +1173,15 @@ pnfs_ld_read_done(struct nfs_read_data *data)
 {
 	int status;
 
-	put_lseg(data->lseg);
-	data->lseg = NULL;
 	if (!data->pnfs_error) {
 		__nfs4_read_done_cb(data);
-		data->mds_ops->rpc_call_done(NULL, data);
+		data->mds_ops->rpc_call_done(&data->task, data);
 		data->mds_ops->rpc_release(data);
 		return 0;
 	}
 
+	put_lseg(data->lseg);
+	data->lseg = NULL;
 	dprintk("%s: pnfs_error=%d, retry via MDS\n", __func__,
 		data->pnfs_error);
 	status = nfs_initiate_read(data, NFS_CLIENT(data->inode), data->mds_ops);
Signature · 1 line
1.7.2.3
#1 lore raw
Quoted 6 lines · Boaz Harrosh

1. In nfs4_write_done_cb: data->write_done_cb comes with a NULL. Just as a guess I call nfs4_write_done_cb() just above it it looked like the right thing to do. With that in, I'm able to write things to file When converting pnfs.c:258 to a WARN_ON.

It is set by the file driver in the .write_pagelist() routine, and if it is not set there, it is set to the default nfs4_write_done_cb in .write_setup(), which ends up being nfs4_proc_write_setup().

Quoted 19 lines

  Benny we might want to set data->write_done_cb somewhere in the none-rpc path? where is it best to do that?
1.5
  Same as above for nfs4_read_done.

2. In pnfs_ld_write_done We don't need:
       put_lseg(data->lseg);
       data->lseg = NULL;
  It is called in nfs_writedata_release()
2.5 Same for pnfs_ld_read_done

3. In pnfs_ld_write_done:
  data->mds_ops->rpc_call_done(NULL, data); crashes with a NULL task. Just pass it with &data->task

  Which calls for a cleanup. There is bunch of functions with [task, write_data] API. And the task is always write_data->task

Quoted 8 lines

3.5
  Same for pnfs_ld_read_done

4. It is now with current code not possible to run with out a *.pg_test* opt. Without, it will crash, reference leak and only do pnfs-IO on a single page. I'll send a patch to check for it in... where the ld->opt is checked.

Yes, this is a bug in the existing code.

Fred

Quoted 131 lines

  So define one for objlayout. Which always returns true.

With this I'm able to read/write with pnfs-IO. (lightly test) (With out any WARN_ONs)

One problem I've seen is that I only get up to 16 pages maximum in a single request. I would like to see the 512 pages we had before for a full request. Where should I fix that?

---
 fs/nfs/nfs4proc.c            |    5 +++-- fs/nfs/objlayout/objio_osd.c |   15 +++++++++++++++ fs/nfs/pnfs.c                |   14 +++++++-------

 3 files changed, 25 insertions(+), 9 deletions(-)
fs/nfs/nfs4proc.c +3 -2
index 759523a..4bf7c0c 100644
@@ -3205,7 +3205,7 @@ static int nfs4_read_done(struct rpc_task *task, struct nfs_read_data *data)
        if (!nfs4_sequence_done(task, &data->res.seq_res))
                return -EAGAIN;
 
-       return data->read_done_cb(task, data);
+       return data->read_done_cb ? data->read_done_cb(task, data) : nfs4_read_done_cb(task, data);
  }
 
  static void nfs4_proc_read_setup(struct nfs_read_data *data, struct rpc_message *msg)
@@ -3250,7 +3250,8 @@ static int nfs4_write_done(struct rpc_task *task, struct nfs_write_data *data)
  {
        if (!nfs4_sequence_done(task, &data->res.seq_res))
                return -EAGAIN;
-       return data->write_done_cb(task, data);
+       return data->write_done_cb ? data->write_done_cb(task, data) :
+               nfs4_write_done_cb(task, data);
  }
 
  /* Reset the the nfs_write_data to send the write to the MDS. */
fs/nfs/objlayout/objio_osd.c +15 -0
index 7e46d2b..105d8a6 100644
@@ -989,6 +989,19 @@ ssize_t objio_write_pagelist(struct objlayout_io_state *ol_state, bool stable)
        return _write_exec(ios);
  }
 
+/*
+ * objlayout_pg_test(). Called by nfs_can_coalesce_requests()
+ *
+ * return 1 :  coalesce page
+ * return 0 :  don't coalesce page
+ */
+int
+objlayout_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev,
+                  struct nfs_page *req)
+{
+       return 1;
+}
+
  static struct pnfs_layoutdriver_type objlayout_type = {
        .id = LAYOUT_OSD2_OBJECTS,
        .name = "LAYOUT_OSD2_OBJECTS",
@@ -1008,6 +1021,8 @@ static struct pnfs_layoutdriver_type objlayout_type = {
 
        .encode_layoutcommit     = objlayout_encode_layoutcommit,
        .encode_layoutreturn     = objlayout_encode_layoutreturn,
+
+       .pg_test                = objlayout_pg_test,
  };
 
  void *objio_init_mt(void)
fs/nfs/pnfs.c +7 -8
index 8d2baf8..ea30a52 100644
@@ -256,7 +256,7 @@ put_lseg_common(struct pnfs_layout_segment *lseg)
  {
        struct inode *inode = lseg->pls_layout->plh_inode;
 
-       BUG_ON(test_bit(NFS_LSEG_VALID, &lseg->pls_flags));
+       WARN_ON(test_bit(NFS_LSEG_VALID, &lseg->pls_flags));
        list_del_init(&lseg->pls_list);
        if (list_empty(&lseg->pls_layout->plh_segs)) {
                set_bit(NFS_LAYOUT_DESTROYED, &lseg->pls_layout->plh_flags);
@@ -1125,15 +1125,15 @@ pnfs_ld_write_done(struct nfs_write_data *data)
  {
        int status;
 
-       put_lseg(data->lseg);
-       data->lseg = NULL;
        if (!data->pnfs_error) {
                pnfs_set_layoutcommit(data);
-               data->mds_ops->rpc_call_done(NULL, data);
+               data->mds_ops->rpc_call_done(&data->task, data);
                data->mds_ops->rpc_release(data);
                return 0;
        }
 
+       put_lseg(data->lseg);
+       data->lseg = NULL;
        dprintk("%s: pnfs_error=%d, retry via MDS\n", __func__,
                data->pnfs_error);
        status = nfs_initiate_write(data, NFS_CLIENT(data->inode), data->mds_ops, NFS_FILE_SYNC);
@@ -1173,15 +1173,15 @@ pnfs_ld_read_done(struct nfs_read_data *data)
  {
        int status;
 
-       put_lseg(data->lseg);
-       data->lseg = NULL;
        if (!data->pnfs_error) {
                __nfs4_read_done_cb(data);
-               data->mds_ops->rpc_call_done(NULL, data);
+               data->mds_ops->rpc_call_done(&data->task, data);
                data->mds_ops->rpc_release(data);
                return 0;
        }
 
+       put_lseg(data->lseg);
+       data->lseg = NULL;
        dprintk("%s: pnfs_error=%d, retry via MDS\n", __func__,
                data->pnfs_error);
        status = nfs_initiate_read(data, NFS_CLIENT(data->inode), data->mds_ops);
--

1.7.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html