Re: [PATCH v2 2/3] fs/ufs: Change the signature of ufs_get_page()

[Fabio M. De Francesco]

On martedì 13 dicembre 2022 08:10:58 CET Al Viro wrote:
> On Tue, Dec 13, 2022 at 12:19:05AM +0100, Fabio M. De Francesco wrote:
> > +static void *ufs_get_page(struct inode *dir, unsigned long n, struct page
> > **page)> 
> >  {
> >  
> >  	struct address_space *mapping = dir->i_mapping;
> > 
> > -	struct page *page = read_mapping_page(mapping, n, NULL);
> > -	if (!IS_ERR(page)) {
> > -		kmap(page);
> > -		if (unlikely(!PageChecked(page))) {
> > -			if (!ufs_check_page(page))
> > +	*page = read_mapping_page(mapping, n, NULL);
> > +	if (!IS_ERR(*page)) {
> > +		kmap(*page);
> > +		if (unlikely(!PageChecked(*page))) {
> > +			if (!ufs_check_page(*page))
> > 
> >  				goto fail;
> >  		
> >  		}
> >  	
> >  	}
> > 
> > -	return page;
> > +	return page_address(*page);
> 
> Er...   You really don't want to do that when you've got ERR_PTR()
> from read_mapping_page().
> 

Sure :-)

Obviously, I'll fix it ASAP.

But I can't yet understand why that pointer was returned unconditionally in 
the code which I'm changing with this patch (i.e., regardless of any potential  
errors from read_mapping_page()) :-/ 

>
> >  fail:
> > -	ufs_put_page(page);
> > +	ufs_put_page(*page);
> > 
> >  	return ERR_PTR(-EIO);
> >  
> >  }
> 
> IDGI...
> 
> static void *ufs_get_page(struct inode *dir, unsigned long n, struct page 
**p)
> {
> 	struct address_space *mapping = dir->i_mapping;
> 	struct page *page = read_mapping_page(mapping, n, NULL);
> 
> 	if (!IS_ERR(page)) {
> 		kmap(page);
> 		if (unlikely(!PageChecked(page))) {
> 			if (!ufs_check_page(page))
> 				goto fail;
> 		}
> 		*p = page;
> 		return page_address(page);
> 	}
> 	return ERR_CAST(page);
> 
> fail:
> 	ufs_put_page(page);
> 	return ERR_PTR(-EIO);
> }
> 
> all there is to it...  The only things you need to change are
> 1) type of function
> 2) make sure to store the page into that pointer to pointer to page on 
success
> 3) return page_address(page) instead of page on success
> 4) use ERR_CAST() to convert ERR_PTR() that is struct page * into equal
> ERR_PTR() that is void * (the last one is optional, just makes the intent
> more clear).

I've never seen anything like this: you are spoon feeding me :-)

Please don't misunderstand: I'm OK with it and, above all, I'm surely  
appreciating how much patience and time you are devolving to help me.

I'll send v3 ASAP (for sure within the next 24 hours).

Furthermore, if there are no other issues, I'd start ASAP with the 
decomposition of the patch to fs/sysv into a series of three units and rework 
the whole design in accordance to the suggestions you have been kindly 
providing.

I'm sorry for responding and reacting so slowly. Aside from being slow because 
of a fundamental lack of knowledge and experience, I can do Kernel development 
(including studying new subjects and code, doing patches, doing reviews, and 
the likes) about 7 hours a week. This is all the time I can set aside until I 
quit one of my jobs at the end of January.

Again thanks,

Fabio

P.S.: While at this patch I'd like to gently ping you about another conversion 
that I sent (and resent) months ago:

[RESEND PATCH] fs/aio: Replace kmap{,_atomic}() with kmap_local_page() at:
https://lore.kernel.org/lkml/20221016150656.5803-1-fmdefrancesco@xxxxxxxxx/

This patch to fs/aio.c has already received two "Reviewed-by" tags: the first 
from Ira Weiny and the second from Jeff Moyer (you won't see the second there, 
because I forgot to forward it when I sent again :-( ).

The tag from Jeff is in the following message (from another [RESEND PATCH] 
thread):
https://lore.kernel.org/lkml/x49h6zzvn1a.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

In that same thread, I explained better I meant in the last sentence of the 
commit message, because Jeff commented that it is ambiguous (I'm adding him in 
the Cc list of recipients).

The original patch, submitted on Thu, 7 Jul 2022 01:33:28 +0200 is at:
https://lore.kernel.org/lkml/20220706233328.18582-1-fmdefrancesco@xxxxxxxxx/

I'd appreciate very much if you can spend some time on that patch too :)