List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:October 1 2010 11:17am
Subject:Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)
Bug#29071
View as plain text  
On Fri, Oct 01, 2010 at 11:46:22AM +0200, Kristofer Pettersson wrote:
> Sergey Vojtovich skrev 2010-10-01 10:36:
> >>>> Please give an example so that I can capture this in the unit test.
> >>> Sure, here they're (please correct me if I'm wrong):
> >>> - my_file_limit became hard limit for open files. For non-win this limit
> is
> >>>   64 by default. In other words applications are now forced to call
> >>>   my_set_max_open_files().
> >> MY_NFILE is defined as 64 but this value was never used since it is
> >> smaller than the lowest possible limit for a default configuration. My
> >> first mistake when making this patch was to use MY_NFILE as an upper
> >> limit and this caused several test files to break.
> > You say that MY_NFILE is never used, but I see it serves two purposes:
> > default value for my_file_limit, min value for my_file_limit.
> > 
> >> From what I can tell my_set_max_open_files() is always called before the
> >> server becomes operational and I think this is now to be considered part
> >> of an IO API.
> > For mysys library users, there is probably a requirement to call my_init(),
> > but I would be very surprised if my_set_max_open_files() were another
> > requirement for applications using IO. I wish sane default here.
> > 
> 
> Currently set_max_open_files() is called unconditionally from
> init_common_variables() which in turn is called unconditionally from
> main() in 5.1 release. In the trunk it is called from mysqld_main()
> (which is still called win_main() in Windows for some bizarre reason).
> 
> In set_max_open_files() the only way to be certain how many max files we
> allow is to check the return value. Despite this we still try to make an
> educated guess by requesting max( max (wanted_files, max_connections*5),
> open_files_limit) where wanted_files is based on obsolete MyISAM
> requirements and open_files_limit is an externally declared variable (or
> 0 if the user didn't supply an open_files_limit).
Server is just one of many users of mysys. :)

> Inside of set_max_open_files() we have the following magic:
>   files+= MY_FILE_MIN;
>   files= set_max_open_files(min(files, OS_FILE_LIMIT));
>   if (files <= MY_NFILE)
>     DBUG_RETURN(files);
> 
> As you can see MY_NFILE is treated as some kind of upper limit. You
> claim it is an lower limit for my_file_limit but I'm not clear on how
> you established this fact. In case of files<=MY_NFILE we don't update
> my_file_limit at all.
Hmm...
Do I interpret the last statement correctly: "my_file_limit cannot be larger
than MY_NFILE"?

My statement was: "my_file_limit cannot be lower than MY_NFILE".

> Also note that my_set_max_open_files() belongs to the mysys library and
> access data which belongs to my_open()/my_fopen() and hence can indeed
> be considered part of the API interfacing that data.
Yes, it is part of the API.

> Now, where does MY_NFILE come in? In the case of my_fopen()? Nowhere.
> MY_NFILE is only mentioned and refelcted on in a really ad hoc comment.
> And in my_open() it isn't visible at all. MY_NFILE is also wrongly
> defined in my_global.h and not mysys.h which would be a more logical
> place. It is used in my_file both as some kind of default value when
> things go bad (and remember that this default value is based on MyISAM
> requirements and has an almost ad hoc value in Windows) and as a strange
> and ad hoc lower limit.
mysys/my_static.c:struct st_my_file_info my_file_info_default[MY_NFILE];
mysys/my_static.c:uint   my_file_limit= MY_NFILE;

> The only thing we need to assert however is that the file descriptor
> maps to the file info array. Everything else is subject to pre- or
> post-tuning, platform specific and should be moved to a higher layer
> where it belongs.
> 
> What other applications you are referring to?
Let's see...
mysqladmin, mysqlbinlog, mysql, mysqlslap, mysqltest, myisamlog, myisamchk,
archive_reader.
Abstract applications linked against libmysqlclient.
Abstract applications linked against mysys.

What about this case:
An application is linked against mysql client library.
An application heavily uses file descriptors, e.g. it has 1000 file descriptors open.
An application requests LOAD DATA LOCAL INFILE.
It fails, because mysql client library by default refuse to work with
file descriptors > 64.

> >>> - in my_fclose(), call to my_win_close() (which is updating
> my_file_info) is
> >>>   not protected by THR_LOCK_open.
> >>
> >> I agree that this is a weak point in my patch which I haven't verified.
> >> I assumed that my_win_fclose() was an equivalent to fclose() and as such
> >> won't contain any data that should be protected by THR_LOCK_open. In
> >> fact if it does (I will check) then we should probably try to change that.
> > Ok.
> 
> You were correct. my_win_fclose() calls
> static void invalidate_fd(File fd)
> {
>   DBUG_ENTER("invalidate_fd");
>   DBUG_ASSERT(fd >= MY_FILE_MIN && fd < (int)my_file_limit);
>   my_file_info[fd].fhandle= 0;
>   DBUG_VOID_RETURN;
> }
> 
> which access the protected data structure. I propose that this is
> violently bad and that this deadly spin of descendant functions should
> be avoided. I'll move the "my_file_info[fd].fhandle= 0;" into my_open()
> / my_fopen() respectively.
> 
> Not holding THR_LOCK_open while performing a system calls seems like a
> good idea to me if it can be avoided.
Agree the purpose of THE_LOCK_open is to protect some variables, but not
system calls. OTOH spreading winfile specific things around code is not
a good idea either.

> fixed.
>
> >>> - did you check if the comment re fileno() still applies? I believe it
> is
> >>>   archaic, it has sense when the condition was like fileno(fd) >
> MY_NFILE.
> >>>   Now it seems useles.
> >> Are you referring to how fileno works for old SUN machines? I left it as
> >> a TODO when we can prove that this is still an issue. I agree that the
> >> comment seems archaic.
> > Revision history is a good proof. This comment justifies type cast:
> > char fileno() returns 10
> > MY_FILENO is 200
> > 
> > (char) 10 is larger than (char) 200
> > (uint) 10 is smaller than (uint) 200
> > 
> > Now we compare int vs uint. Comment is obsolete.
> > 
> I have no idea how you connect the comment. I used the old code, but I
> agree that this is better:
> 
>   file= my_fileno(fd);
>   if ((uint) file >= (uint) my_file_limit)
>   {
>     /* Function called with wrong parameters */
>     DBUG_RETURN(EBADF);
>   }
> 
> Fixed.
I can always elaborate. What's wrong with my explanation?

> >>> - there are a couple of redundant checks introduced with this patch
> >>>   and our rule was always to avoid redundancy (e.g. my_fclose() checks
> >>>   if fd is not null).
> >> I think it is good that a function validates its parameters. Where is
> >> the redundancy? How would you like it to be?
> > Talking particulary about my_fclose() check. Why would one want to call
> > my_fclose(NULL)? I'd rather expect broken or closed FILE structure, but
> > you cannot validate it in this case.
> > 
> Yes, in a call to std fclose() or fileno() using a NULL argument would
> crash the server. I suppose this is a question which depends on how you
> envision the my_close() functions to be used. I think we should allow
> for multiple close and not crash on invalid arguments but rather treat
> them with caution. However, I'm not going to be stubborn about this and
> if you insist that we should crash on NULL values I can remove this check.
> 
> Done.
>
> >>>>> For this particular bug I'd prefer to see a one-line fix in
> my_close().
> >>>>> I'm ok if you want to fix my_strdup() issue as well, but this is
> yet
> >>>>> another one-line fixes.
> >>>>
> >> I know you do. I would how ever not be very satisfied with a yet another
> >> patch on this already heavy patchwork. Better to get a clean slate and
> >> create something to be proud of.
> > Ok, I believe I wasn't clear enough, my point is: I find that the result
> > of this refactoring is different code. Not worse, not better - just
> > different.
> > 
> Right. Lets work with it until we're both satisfied. I think it is
> better because in the end at least two people will fully understand
> what's going on.
> 
> >>>
> >> Which corrections did you make to the test case?
> > One was correction: avoid my_fclose(NULL);
> > Second was fix: avoid multiple my_fclose() on the same file descriptor (at
> least
> > not permitted according to fclose manual).
> 
> I argue that we should allow for multiple closes and NULL arguments.
> Copying the bare bone std calls doesn't make much sense. Especially not
> when we must avoid crashing the server under all circumstances.
Would you do the same for other calls like read/write/fileno/etc?

Regards,
Sergey
-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
Thread
bzr commit into mysql-trunk branch (kristofer.pettersson:3197) Bug#29071Kristofer Pettersson27 Sep
Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Kristofer Pettersson29 Sep
  • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich30 Sep
    • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Kristofer Pettersson30 Sep
      • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich1 Oct
Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich29 Sep
Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Kristofer Pettersson1 Oct
  • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich1 Oct
    • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Kristofer Pettersson1 Oct
      • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich1 Oct
        • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Kristofer Pettersson4 Oct