diff mbox series

[1/1] pNFS/filelayout: check layout segment range

Message ID 20240507195933.45683-1-olga.kornievskaia@gmail.com (mailing list archive)
State New
Headers show
Series [1/1] pNFS/filelayout: check layout segment range | expand

Commit Message

Olga Kornievskaia May 7, 2024, 7:59 p.m. UTC 
From: Olga Kornievskaia <kolga@netapp.com>

Before doing the IO, check that we have the layout covering the range of
IO.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/filelayout/filelayout.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Benjamin Coddington May 8, 2024, 2:50 p.m. UTC | #1 
On 7 May 2024, at 15:59, Olga Kornievskaia wrote:

> From: Olga Kornievskaia <kolga@netapp.com>
>
> Before doing the IO, check that we have the layout covering the range of
> IO.
>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/filelayout/filelayout.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
> index 85d2dc9bc212..bf3ba2e98f33 100644
> --- a/fs/nfs/filelayout/filelayout.c
> +++ b/fs/nfs/filelayout/filelayout.c
> @@ -868,6 +868,7 @@ filelayout_pg_init_read(struct nfs_pageio_descriptor *pgio,
>  			struct nfs_page *req)
>  {
>  	pnfs_generic_pg_check_layout(pgio);
> +	pnfs_generic_pg_check_range(pgio, req);
>  	if (!pgio->pg_lseg) {
>  		pgio->pg_lseg = fl_pnfs_update_layout(pgio->pg_inode,
>  						      nfs_req_openctx(req),
> @@ -892,6 +893,7 @@ filelayout_pg_init_write(struct nfs_pageio_descriptor *pgio,
>  			 struct nfs_page *req)
>  {
>  	pnfs_generic_pg_check_layout(pgio);
> +	pnfs_generic_pg_check_range(pgio, req);
>  	if (!pgio->pg_lseg) {
>  		pgio->pg_lseg = fl_pnfs_update_layout(pgio->pg_inode,
>  						      nfs_req_openctx(req),
> -- 
> 2.39.1

Looks right, less duplication to just call pnfs_generic_pg_check_range()
from pnfs_generic_pg_check_layout() now.

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben
Olga Kornievskaia May 8, 2024, 5:52 p.m. UTC | #2 
On Wed, May 8, 2024 at 10:50 AM Benjamin Coddington <bcodding@redhat.com> wrote:
>
> On 7 May 2024, at 15:59, Olga Kornievskaia wrote:
>
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Before doing the IO, check that we have the layout covering the range of
> > IO.
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfs/filelayout/filelayout.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
> > index 85d2dc9bc212..bf3ba2e98f33 100644
> > --- a/fs/nfs/filelayout/filelayout.c
> > +++ b/fs/nfs/filelayout/filelayout.c
> > @@ -868,6 +868,7 @@ filelayout_pg_init_read(struct nfs_pageio_descriptor *pgio,
> >                       struct nfs_page *req)
> >  {
> >       pnfs_generic_pg_check_layout(pgio);
> > +     pnfs_generic_pg_check_range(pgio, req);
> >       if (!pgio->pg_lseg) {
> >               pgio->pg_lseg = fl_pnfs_update_layout(pgio->pg_inode,
> >                                                     nfs_req_openctx(req),
> > @@ -892,6 +893,7 @@ filelayout_pg_init_write(struct nfs_pageio_descriptor *pgio,
> >                        struct nfs_page *req)
> >  {
> >       pnfs_generic_pg_check_layout(pgio);
> > +     pnfs_generic_pg_check_range(pgio, req);
> >       if (!pgio->pg_lseg) {
> >               pgio->pg_lseg = fl_pnfs_update_layout(pgio->pg_inode,
> >                                                     nfs_req_openctx(req),
> > --
> > 2.39.1
>
> Looks right, less duplication to just call pnfs_generic_pg_check_range()
> from pnfs_generic_pg_check_layout() now.

filelayout.c is not the only caller of pnfs_generic_pg_check_layout().
flexfilelayout has a wrapper around those 2 functions and calls that.
however, the argument about duplicated code frustrates me because
currently the code has 4lines. but if we were to re-write the same
with a function, it would be more lines used in total (flexfiles has 8
lines for it).

>
> Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
>
> Ben
>
Olga Kornievskaia May 8, 2024, 5:55 p.m. UTC | #3 
On Wed, May 8, 2024 at 1:52 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
>
> On Wed, May 8, 2024 at 10:50 AM Benjamin Coddington <bcodding@redhat.com> wrote:
> >
> > On 7 May 2024, at 15:59, Olga Kornievskaia wrote:
> >
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > >
> > > Before doing the IO, check that we have the layout covering the range of
> > > IO.

I should add that this patch extends previously posted 2 patches by
Anna for the partial layout support for filelayout (as in they go
together).

> > >
> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > ---
> > >  fs/nfs/filelayout/filelayout.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
> > > index 85d2dc9bc212..bf3ba2e98f33 100644
> > > --- a/fs/nfs/filelayout/filelayout.c
> > > +++ b/fs/nfs/filelayout/filelayout.c
> > > @@ -868,6 +868,7 @@ filelayout_pg_init_read(struct nfs_pageio_descriptor *pgio,
> > >                       struct nfs_page *req)
> > >  {
> > >       pnfs_generic_pg_check_layout(pgio);
> > > +     pnfs_generic_pg_check_range(pgio, req);
> > >       if (!pgio->pg_lseg) {
> > >               pgio->pg_lseg = fl_pnfs_update_layout(pgio->pg_inode,
> > >                                                     nfs_req_openctx(req),
> > > @@ -892,6 +893,7 @@ filelayout_pg_init_write(struct nfs_pageio_descriptor *pgio,
> > >                        struct nfs_page *req)
> > >  {
> > >       pnfs_generic_pg_check_layout(pgio);
> > > +     pnfs_generic_pg_check_range(pgio, req);
> > >       if (!pgio->pg_lseg) {
> > >               pgio->pg_lseg = fl_pnfs_update_layout(pgio->pg_inode,
> > >                                                     nfs_req_openctx(req),
> > > --
> > > 2.39.1
> >
> > Looks right, less duplication to just call pnfs_generic_pg_check_range()
> > from pnfs_generic_pg_check_layout() now.
>
> filelayout.c is not the only caller of pnfs_generic_pg_check_layout().
> flexfilelayout has a wrapper around those 2 functions and calls that.
> however, the argument about duplicated code frustrates me because
> currently the code has 4lines. but if we were to re-write the same
> with a function, it would be more lines used in total (flexfiles has 8
> lines for it).
>
> >
> > Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
> >
> > Ben
> >
Benjamin Coddington May 8, 2024, 6:23 p.m. UTC | #4 
On 8 May 2024, at 13:52, Olga Kornievskaia wrote:

> On Wed, May 8, 2024 at 10:50 AM Benjamin Coddington <bcodding@redhat.com> wrote:
>>
>> On 7 May 2024, at 15:59, Olga Kornievskaia wrote:
>>
>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>
>>> Before doing the IO, check that we have the layout covering the range of
>>> IO.
>>>
>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>> ---
>>>  fs/nfs/filelayout/filelayout.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
>>> index 85d2dc9bc212..bf3ba2e98f33 100644
>>> --- a/fs/nfs/filelayout/filelayout.c
>>> +++ b/fs/nfs/filelayout/filelayout.c
>>> @@ -868,6 +868,7 @@ filelayout_pg_init_read(struct nfs_pageio_descriptor *pgio,
>>>                       struct nfs_page *req)
>>>  {
>>>       pnfs_generic_pg_check_layout(pgio);
>>> +     pnfs_generic_pg_check_range(pgio, req);
>>>       if (!pgio->pg_lseg) {
>>>               pgio->pg_lseg = fl_pnfs_update_layout(pgio->pg_inode,
>>>                                                     nfs_req_openctx(req),
>>> @@ -892,6 +893,7 @@ filelayout_pg_init_write(struct nfs_pageio_descriptor *pgio,
>>>                        struct nfs_page *req)
>>>  {
>>>       pnfs_generic_pg_check_layout(pgio);
>>> +     pnfs_generic_pg_check_range(pgio, req);
>>>       if (!pgio->pg_lseg) {
>>>               pgio->pg_lseg = fl_pnfs_update_layout(pgio->pg_inode,
>>>                                                     nfs_req_openctx(req),
>>> --
>>> 2.39.1
>>
>> Looks right, less duplication to just call pnfs_generic_pg_check_range()
>> from pnfs_generic_pg_check_layout() now.
>
> filelayout.c is not the only caller of pnfs_generic_pg_check_layout().
> flexfilelayout has a wrapper around those 2 functions and calls that.
> however, the argument about duplicated code frustrates me because
> currently the code has 4lines. but if we were to re-write the same
> with a function, it would be more lines used in total (flexfiles has 8
> lines for it).

sorry - not trying to frustrate.  Since everyone is now calling both
pnfs_generic_pg_check_layout /and/ pnfs_generic_pg_check_range in the same
place, I thought you could just put
!pnfs_lseg_request_intersecting(pgio->pg_lseg, req) in the first test within
pnfs_generic_pg_check_layout(pgio, req), making that function do the work of
both.  Then you can delete pnfs_generic_pg_check_range entirely for
everyone.  The name still makes sense.

Ben
diff mbox series

Patch

diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
index 85d2dc9bc212..bf3ba2e98f33 100644
--- a/fs/nfs/filelayout/filelayout.c
+++ b/fs/nfs/filelayout/filelayout.c
@@ -868,6 +868,7 @@  filelayout_pg_init_read(struct nfs_pageio_descriptor *pgio,
 			struct nfs_page *req)
 {
 	pnfs_generic_pg_check_layout(pgio);
+	pnfs_generic_pg_check_range(pgio, req);
 	if (!pgio->pg_lseg) {
 		pgio->pg_lseg = fl_pnfs_update_layout(pgio->pg_inode,
 						      nfs_req_openctx(req),
@@ -892,6 +893,7 @@  filelayout_pg_init_write(struct nfs_pageio_descriptor *pgio,
 			 struct nfs_page *req)
 {
 	pnfs_generic_pg_check_layout(pgio);
+	pnfs_generic_pg_check_range(pgio, req);
 	if (!pgio->pg_lseg) {
 		pgio->pg_lseg = fl_pnfs_update_layout(pgio->pg_inode,
 						      nfs_req_openctx(req),